-
Couldn't load subscription status.
- Fork 109
feat: introduce templates for vaults upgrade and revenue share opt in #1257
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
base: main
Are you sure you want to change the base?
feat: introduce templates for vaults upgrade and revenue share opt in #1257
Conversation
chore: sync branch revshare
|
|
||
| Install supersim if you haven't already: | ||
| ```bash | ||
| # Installation instructions: https://github.com/ethereum-optimism/supersim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely get the value of this, though we need to understand what we're asking signers to install and run. New dependencies tend to present challenges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am interpreting this as a way to test end to end rather than what the signers do to validate. Do we have prior art for validation of deposit transactions using tenderly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, it's a way to test the deposits when they arrive on L2, the documentation is only for developers.
Should we move the file to the integration tests folder instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is worth mentioning that these tests were very useful for us. We could ensure that all the batch of deposits worked as expected, leaving the assertions on the code so anyone can run them and verify the script works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have prior art for validation of deposit transactions using tenderly?
Not to my knowledge, am asking tho.
I can see the value of these tests for devs, let's just leave as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, prior art is here: https://github.com/ethereum-optimism/superchain-ops/blob/49dde035aafe7d0ce6245a58461c6eef54206217/src/doc/simulate-l2-ownership-transfer.md
TLDR:
- simulate the task
- grab the arguments emitted by the
TransactionDepositedevent - create a new simulation against the L2 with the arguments
- validate that simulation
I don't think that process is scaleable enough to be applied here unfortunately.
|
|
||
| ### Step 3: Add State Assertions | ||
|
|
||
| After relaying messages, assert that the L2 state matches expectations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the assertions just be handled in the template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the assertions is over the L2 state when the deposits arrive, and the template is the script for creating and triggering the deposits on L1
| # Revenue Share Simulations | ||
| A list of tenderly simulations are provided based on the formatting of the `TransactionDeposited` events emitted in L1 and its execution in the target L2, in this case, OP Mainnet. For details, check the [Simulating Portal Deposit Transactions](todo) document | ||
|
|
||
| ### Revenue Share Upgrade Opt In |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot. Is the plan for signers to validate all of these?
And these are all executed as deposit transactions? Could the deployments (at least) just be submitted to the sequencer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this is a lot, but we need all those txs for setting up the chain correctly. Bear in mind that each chain will either chose one upgrade path or another (opt in vs late opt in).
And these are all executed as deposit transactions?
Yes!
Could the deployments (at least) just be submitted to the sequencer?
Not sure if I followed here. We are using CREATE2_DEPLOYER so we get the deployments over deterministic addresses and we are able to set up them at once on the template that triggers deposits. But let me know if this was your question, or if it was something else!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that these transactions currently go: deploy, upgrade, deploy, upgrade, etc.
So I'm wondering if instead we could:
- deploy all the contracts we need
- validate them somehow
- perform the upgrade transactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understood your point, you're proposing splitting the current template into:
- Deposits for deployments
- Deposits for upgrades
And manually verifying that all the deployments were correctly done before executing the upgrades?
Lemme know if I'm following you right. If this is the way you're thinking, I think it adds even more overhead to who is executing the script, and adds more room for human error, since now you'll also need to receive the implementation addresses as inputs instead of having them precalculated.
Current integration tests are very useful to check that deployments went good, and also for the final state of proxies after the upgrade and initialization. Nevertheless, happy to discuss if you think the approach we initially thought is not the ideal one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the newly deployed contracts have constructor arguments? Part of my thought is that validating the input addresses (as you mentioned) might be easier if they are all deployed to the same addresses on each chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Yes, L1Withdrawer and SuperchainRevSharesCalculator both have constructor arguments:
SuperchainRevSharesCalculator: shareRecipient, remainderRecipient (different per chain)L1Withdrawer: recipient, minWithdrawalAmount, withdrawalGasLimit (different per chain)
Given this constraint, these contracts will be deployed to different addresses for each chain due to different inputs.
| ```bash | ||
| # Installation instructions: https://github.com/ethereum-optimism/supersim | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see any reason to make this a bash comment?
| ```bash | |
| # Installation instructions: https://github.com/ethereum-optimism/supersim | |
| ``` | |
| Installation instructions: https://github.com/ethereum-optimism/supersim |
Description
This PR contains 2 new templates, covering 3 possible paths:
Additionally, it provides a template for deploying the
FeesDepositorcontract on L1. After executing this action, some updates are needed ethereum-optimism/optimism#17505.Tests
We simulated and manually checked the deposits on Tenderly. Also, we added comprehensive unit test coverage, and most importantly we wrote some integration tests using Supersim to check the state in L2 when deposits arrive there.
Additional context
The integration test setup also provides the tenderly links to check for the simulations
Metadata
FeesDepositoraddress on deployer upon deployment optimism#17505.Authors
@0xChin @0xiamflux