Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GovernanceDelegation Interop #54

Merged
merged 9 commits into from
Aug 12, 2024

Conversation

corydickson
Copy link
Contributor

Description

This design doc includes details about a potential update to the Governance Delegation contract allowing for
interoperability of partial delegations.

@tynes
Copy link
Contributor

tynes commented Jul 30, 2024

We will schedule this for design review on Thursday

@tynes tynes mentioned this pull request Jul 30, 2024

## Rationale

Due to the fact that the `GovernanceDelegation` contract now has awareness of the block numbers and chain IDs of all of the voting power checkpoints each L2 can now be responsible for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this awareness a bit? I see some mention of it in the overview. It could be useful to have some simple data structures in here to make it easier to visualize.

Also i would like to avoid branching logic based on the chainid. It would be most ideal if the events could be imported into any chain but with no real incentive to import them anywhere besides op mainnet. This will simplify testing, remove edge cases and enable delegation power to be observed from any chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was unsure if pseudocode was bad style for a design doc, but specifically I was considering some type of mapping in solidity between mapping ( uint16 => Checkpoint ) where the data structure is ERC20Votes.Checkpoint[] contains an additional field blockNumber


## Implementation

Again, the implementation involves updates the Governor that will ensure that for voting power that has been migrated can be retrieved and updated based on the state on each of the L2 chains. It accomplishes this by keeping track of the token nonce and block numbers of each checkpoint in the mainnet contract. Instead of directly sending messages between the different instances, Governors on non-OP mainnet chains will now emit events regarding these blocks in the their checkpoint updates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say Governor, do you mean GovToken?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Governor contract is an additional contract that already exists to tally up voting power, essentially. It's what handles voting outside of the GovToken contract. It makes the most sense to have the Governor be responsible for tallying up voting power for a particular vote.

When originally considering this problem, the idea was to create a hook during the `afterTokenTransfer` event. As alluded to above, this do not provide some of the desired functionality.It also makes a few assumptions about the behaviors of delegates that receive tokens and stifles their ability to create partial delegations in a low-cost manner. Moreover, if there was
a want to later batch updates of voting power to mainnet it would be costly to those operating governance.

Another active consideration is the use of block timestamp of each L2 instead of block number while implementing this new solution. The primary reason for block numbers to be used in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to determine if we're using L1 block number or timestamp.

Main reason to use L1 block number is that it makes the process of using L1 => L2 transactions for voting very easy to think about. This is important in the censorship case. If you want your vote to count, you know that you must include it in an L1 => L2 transaction by L1 block N.

Main reason to use timestamp is that it scales to chains where the L1 block number is not available in the same way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a noob question, but what do you mean the "L1 block number is not available in the same way"?

2. Indexers must now accept a delay in updates if wishing to give an accurate voting status for a particular delegate.
3. Minimize the griefing vector of incurring costs onto delegators.
4. Estimation of costs of operating relayers to ensure they are still incentivized to relay messages.
5. Batch ordering of checkpoints across all L2 chains.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm not sure what this means, can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this means to batch process the checkpoints on each L2 to reduce frequency & cost of updates (especially when multiple small transfers occur). At least I think that is what it means, and would like to see that as part of the design. :) I feel like this may be tricky though since batching could delay the update to Governor on mainnet maybe?

Copy link
Contributor Author

@corydickson corydickson Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this would be exposing some batching function to process the messages in the 'cross-message passing Inbox'

@skeletor-spaceman
Copy link
Contributor

@corydickson
We've re-read the document a few times and it's still a bit confusing.
It'd be great to have a figma or detailed flows on how you are envisioning the technical implementation being.

When originally considering this problem, the idea was to create a hook during the `afterTokenTransfer` event. As alluded to above, this do not provide some of the desired functionality.It also makes a few assumptions about the behaviors of delegates that receive tokens and stifles their ability to create partial delegations in a low-cost manner. Moreover, if there was
a want to later batch updates of voting power to mainnet it would be costly to those operating governance.

Another active consideration is the use of block timestamp of each L2 instead of block number while implementing this new solution. The primary reason for block numbers to be used in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a noob question, but what do you mean the "L1 block number is not available in the same way"?

2. Indexers must now accept a delay in updates if wishing to give an accurate voting status for a particular delegate.
3. Minimize the griefing vector of incurring costs onto delegators.
4. Estimation of costs of operating relayers to ensure they are still incentivized to relay messages.
5. Batch ordering of checkpoints across all L2 chains.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this means to batch process the checkpoints on each L2 to reduce frequency & cost of updates (especially when multiple small transfers occur). At least I think that is what it means, and would like to see that as part of the design. :) I feel like this may be tricky though since batching could delay the update to Governor on mainnet maybe?

governance/delegation-interop.md Show resolved Hide resolved
Again, the implementation involves updates the Governor that will ensure that for voting power that has been migrated can be retrieved and updated based on the state on each of the L2 chains. It accomplishes this by keeping track of the token nonce and block numbers of each checkpoint in the mainnet contract. Instead of directly sending messages between the different instances, Governors on non-OP mainnet chains will now emit events regarding these blocks in the their checkpoint updates.

The new storage values will be used to keep track of both the voting power that has been accumulated on the native chain as well as others thus not breaking backwards compatibility for
non-migrated tokens. Indexers will now need to be aware of the events being emitted on both OP mainnet and the other L2's that implement the `GovToken` to show checkpoints that have yet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So indexers need to watch every possible L2 that has governance token delegation? Just trying to understand the flow:

  1. Event happens on L2 chain, changing delegation.
  2. GovToken contract emits event in its checkpoint update.
  3. Governor contract is aware of checkpoint updates(I'm not totally clear on how this works, is it using interop messaging to do this?)
  4. Governor updates state and checkpoints and this info is indexed.
  5. But indexer is also listening for the L2 event emission because it should know about checkpoints from all L2s even if they are not on mainnet yet?

What happens if there is a delegation change on an L2 but it hasn't yet been submitted to mainnet? What is the source of truth if there is a vote in that scenario? It could be problematic if the onchain source of truth doesn't match the indexer/offchain source of truth, right? I think some sequence diagrams would be helpful here to visualize the various components and how they interact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will include some diagrams in the document. But your understanding is correct, the source of truth is always the L2 but the block number inclusion allows votes to be counted on Mainnet for up to N specified blocks.

@@ -0,0 +1,65 @@
# Purpose

The primary purpose of this design document is to give an approach to support voting power delegation across multiple L2's that employ the `GovernanceDelegation` and `GovToken` as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I haven't been fully in the interop look:

Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct I'll be supplying links

Comment on lines 18 to 19
However, pushing all of the logic onto the `GovToken` proves to be problematic given the potential flood of messages that may be out of sequence or have many smaller transfer events
prior to a single delegation event. The following section aims to outline a solution to correct this as well as ensure that relayers still have the proper incentives to handle these
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this problem? I don't follow the "problematic given the potential flood of messages that may be out of sequence or have many smaller transfer events" aspect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here was to provide some context around the previous solution which had a hook afterTokenTransfer in which a message was sent between L2's to OP main net that then updated the checkpoints. This is no longer pertinent to the design but was trying to lay the groundwork of some prior thinking around supporting this feature. Here's the PR of the old spec that it looks like we are moving away from: ethereum-optimism/specs#285

Comment on lines 24 to 25
When originally considering this problem, the idea was to create a hook during the `afterTokenTransfer` event. As alluded to above, this do not provide some of the desired functionality.It also makes a few assumptions about the behaviors of delegates that receive tokens and stifles their ability to create partial delegations in a low-cost manner. Moreover, if there was
a want to later batch updates of voting power to mainnet it would be costly to those operating governance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would this afterTokenTransfer event have done? Would be great to expand on the alternatives in more detail for readers that may not have the full context (like me 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted the link in the previous comment, I'll be sure to coalesce all the existing documentation here.

governance/delegation-interop.md Show resolved Hide resolved
@cairoeth cairoeth linked an issue Jul 31, 2024 that may be closed by this pull request
Copy link
Contributor

@cairoeth cairoeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some suggestions to clarify the first sections with more specific context and target problem

governance/delegation-interop.md Outdated Show resolved Hide resolved
governance/delegation-interop.md Outdated Show resolved Hide resolved
corydickson and others added 2 commits August 1, 2024 13:33
Co-authored-by: cairo <cairoeth@protonmail.com>
Co-authored-by: cairo <cairoeth@protonmail.com>
@tynes
Copy link
Contributor

tynes commented Aug 1, 2024

We are going to want a design doc for Governor

@skeletor-spaceman
Copy link
Contributor

I'd love it if we could include a section of the potential drawbacks and added complexity of this approach.
Since we are tracking the digested voting power on each chain separately, does this mean that as a token holder, I would need to re-execute the delegate(...) function on each chain to be sure those tokens are being delegated as well?
and if I choose to change them, i'd need to do so on all chains as well?

it'd be great to know which potential pitfalls or complexities we might encounter as we implement and integrate this.

@tynes
Copy link
Contributor

tynes commented Aug 7, 2024

Will schedule this for design review on Monday

@tynes tynes mentioned this pull request Aug 7, 2024

# Summary

There will be two main changes to the existing `GovernanceDelegation` [contract](https://github.com/ethereum-optimism/specs/blob/main/specs/experimental/gov-delegation.md) to support interoperability. First, the function in which the checkpoints can be queried from the contract will now check for the migration status prior to retrieving voting power and implement an announce function that will emit an event that contains the chain ID and the block number of the latest checkpoint. The contract on OP mainnet will then consume these events, and in its own storage provide a mapping between chainIds and an object containing both the latest checkpoint and the block number. After tokens are transferred using the `GovernanceToken` [contract](https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/governance/GovernanceToken.sol) a check is performed on the delegation contract that ensures that before this mapping is updated, the nonce of the token transfer received has been incremented as well as being sent from a block number greater than the current one.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the function in which the checkpoints can be queried from the contract will now check for the migration status prior to retrieving voting power and implement an announce function that will emit an event that contains the chain ID and the block number of the latest checkpoint

I am confused about this, generally a query implies a view function (ie STATICCALL) meaning no state changes allowed, including events being emitted

Copy link
Contributor

@tynes tynes Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract on OP mainnet will then consume these events, and in its own storage provide a mapping between chainIds and an object containing both the latest checkpoint and the block number

Which contract? The GovToken, GovernanceDelegation or Governor? Also we should design it in a generic way so that the checkpoints can be accumulated to any chain, this will make everything easier to reason about

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a check is performed on the delegation contract that ensures that before this mapping is updated, the nonce of the token transfer received has been incremented as well as being sent from a block number greater than the current one.

Does this imply that users cannot transfer if they first do not relay events from the remote chain?

@tynes tynes merged commit d40a730 into ethereum-optimism:main Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GovDelegation: Interop Design Doc
7 participants