-
Notifications
You must be signed in to change notification settings - Fork 581
feat(avm): protocol contractg mutations #19586
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
feat(avm): protocol contractg mutations #19586
Conversation
ed25916 to
23929ad
Compare
23aabe7 to
1054464
Compare
23929ad to
ed98aa8
Compare
1054464 to
3bc2633
Compare
ed98aa8 to
5864a4d
Compare
3bc2633 to
2584a41
Compare
| protected contractsDB: PublicContractsDB, | ||
| protected globalVariables: GlobalVariables, | ||
| config?: Partial<PublicSimulatorConfig>, | ||
| protected protocolContracts: ProtocolContracts = ProtocolContractsList, |
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.
needed to change the interface because we would otherwise force to read from the list
| // MIN_FEE must be >= 1 to prevent underflow in compute_effective_gas_fees, since | ||
| // global_variables.gas_fees is hardcoded to {1, 1}. This can change once we enable | ||
| // smart mutations of global variables that maintain the invariant max_fees_per_gas >= gas_fees. | ||
| // MIN_FEE must be >= 1 to prevent underflow in compute_effective_gas_fees. |
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 comment was missed when we mutated hte global variables in an earlier pr
|
|
||
| namespace { | ||
|
|
||
| void update_enqueued_calls_for_protocol_contract(Tx& tx, |
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 bit of a code-smell, but we need to update the addresses used in the enqueued calls whenever we modify the protocol contracts and we would have potentially invalidated addresses that are referenced by enqueued calls
| // For protocol contracts, also add instances keyed by canonical address (1-11). | ||
| // This is needed because protocol contracts are looked up by canonical address, | ||
| // but the derived address in protocol_contracts.derived_addresses maps to the actual instance. | ||
| for (size_t i = 0; i < tx_data.protocol_contracts.derived_addresses.size(); ++i) { |
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 so that when we look up the protocol contracts we can retrieve the instance
| break; | ||
| } | ||
|
|
||
| // Clear any protocol contract derived addresses that reference addresses no longer in the contract set. |
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 pretty lazy, but i was running into some issues with calling stale contract addresses. I'll investigate a cleaner way to ensure we dont need a clean up step after mutations.
5864a4d to
a21f4d8
Compare
2584a41 to
8b90735
Compare
4c7af27 to
2c157cb
Compare
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
2c157cb to
16c9a69
Compare

Protocol contract mutations turned out to be much more complex. Might've been easier to implement a hardcoded set.
We need to ensure we re-validate the enqueued calls whenever we mutate the protocol contracts since we could have invalidated some addresses.
Note: The TS simulation required a change to match the cpp simulator