-
Notifications
You must be signed in to change notification settings - Fork 26
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
make dispatcher reinitializable + add initilization to upgrade specs #134
Conversation
WalkthroughThe modifications involve enhancing initialization logic by adding explicit visibility modifiers, updating reinitializer values, and modifying related scripts. This includes restructuring various parameter handling elements and introducing new functions for encoding initialization arguments, enhancing the clarity and functionality of the initialization processes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dispatcher
participant Deployer
participant ExternalContract
User->>Deployer: Initiate Deployment
Deployer->>Dispatcher: initialize(initPortPrefix, _feeVault)
Dispatcher-->>User: Initialization Complete
User->>Dispatcher: Send Transaction (with init)
Dispatcher->>ExternalContract: renderArgs(init.args, init.signature)
ExternalContract-->>Dispatcher: Response
Dispatcher-->>User: Transaction Sent
Poem
Tip AI model upgrade
|
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.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- contracts/core/Dispatcher.sol (1 hunks)
- src/deploy.ts (1 hunks)
- src/evm/contracts/factories/Dispatcher__factory.ts (1 hunks)
- src/evm/schemas/tx.ts (1 hunks)
- src/tx.ts (1 hunks)
- src/utils/io.ts (2 hunks)
Files not summarized due to errors (1)
- src/evm/contracts/factories/Dispatcher__factory.ts: Error: Message exceeds token limit
Additional comments not posted (6)
src/evm/schemas/tx.ts (1)
14-19
: Add documentation for the newinit
field.The new
init
field in theTxItemSchema
should be documented to clarify its purpose and usage.// Initialization data for the transaction, including the signature and arguments. init: z.optional( z.object({ signature: z.string().min(1), args: z.array(z.string().min(1)), }) ),src/tx.ts (1)
78-79
: Verify the correctness ofrenderArgs
usage withtx.init
.Ensure that the
tx.init
parameter is correctly handled in therenderArgs
function and that it does not introduce any side effects.Verification successful
Verified the correctness of
renderArgs
usage withtx.init
.The
renderArgs
function correctly handles thetx.init
parameter by compiling initial arguments and replacing$INITARGS
in theargs
array without introducing any side effects.
- The implementation of
renderArgs
is located insrc/utils/io.ts
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `renderArgs` with the `tx.init` parameter. # Test: Search for the `renderArgs` function implementation and verify its handling of `tx.init`. rg --type typescript $'renderArgs'Length of output: 68
Script:
#!/bin/bash # Description: Verify the usage of `renderArgs` with the `tx.init` parameter. # Test: Search for the `renderArgs` function implementation and verify its handling of `tx.init`. rg --type ts 'renderArgs'Length of output: 335
Script:
#!/bin/bash # Description: Extract the implementation of the `renderArgs` function from `src/utils/io.ts`. # Extract the `renderArgs` function implementation. ast-grep --lang typescript --pattern $'export const renderArgs = ($_$) => { $$$ }' src/utils/io.tsLength of output: 101
Script:
#!/bin/bash # Description: Extract the implementation of the `renderArgs` function from `src/utils/io.ts`. # Search for the `renderArgs` function definition and extract its implementation. rg 'renderArgs' -A 20 src/utils/io.tsLength of output: 557
src/deploy.ts (1)
52-52
: Verify the correctness ofrenderArgs
usage withinit
.Ensure that the
init
parameter is correctly handled in therenderArgs
function withingetDeployData
.Verification successful
Verified the correctness of
renderArgs
usage withinit
.The
init
parameter is correctly handled in therenderArgs
function, ensuring proper integration and functionality.
src/utils/io.ts
: lines 282-300Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `renderArgs` with the `init` parameter in `getDeployData`. # Test: Search for the `renderArgs` function implementation and verify its handling of `init`. rg --type typescript $'renderArgs'Length of output: 68
Script:
#!/bin/bash # Description: Verify the usage of `renderArgs` with the `init` parameter in `getDeployData`. # Test: Search for the `renderArgs` function implementation and verify its handling of `init`. ast-grep --lang typescript --pattern $'function renderArgs($_, $_, $_) { $$$ }'Length of output: 82
Script:
#!/bin/bash # Description: Verify the usage of `renderArgs` with the `init` parameter in `getDeployData`. # Test: Search for all occurrences of `renderArgs` in the codebase to locate the function definition. rg 'renderArgs'Length of output: 325
Script:
#!/bin/bash # Description: Extract the implementation of the `renderArgs` function from `src/utils/io.ts` to verify its handling of the `init` parameter. # Test: Extract the `renderArgs` function implementation. ast-grep --lang typescript --pattern $'export const renderArgs = ($_,$_,$_) => { $$$ }' src/utils/io.tsLength of output: 1001
contracts/core/Dispatcher.sol (1)
80-85
: LGTM! Verify the usage ofreinitializer(2)
.The use of
reinitializer(2)
is appropriate for allowing reinitialization with a new version number.Ensure that all necessary checks and balances are in place for reinitialization.
Verification successful
Usage of
reinitializer(2)
verified and appropriate.The
reinitializer(2)
modifier is correctly used incontracts/core/Dispatcher.sol
to allow reinitialization with a new version number. This ensures proper handling of contract upgrades.
- Location:
contracts/core/Dispatcher.sol
, line 83Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `reinitializer` in the codebase. # Test: Search for the usage of `reinitializer`. Expect: Only occurances with appropriate version numbers. rg --type solidity $'reinitializer'Length of output: 67
Script:
#!/bin/bash # Description: Verify all occurrences of `reinitializer` in the Solidity codebase. # Use `fd` to find all Solidity files and then search for `reinitializer` in those files. fd -e sol -x grep -Hn 'reinitializer' {}Length of output: 7973
src/evm/contracts/factories/Dispatcher__factory.ts (2)
Line range hint
14-14
: Ensure consistent formatting for autogenerated files.The comment indicates the file is autogenerated. Ensure that any manual changes are consistently applied across the project to maintain uniformity.
2114-2114
: Update_bytecode
variable to reflect new contract changes.The
_bytecode
variable has been updated. Ensure that this change corresponds to the latest contract deployment bytecode.
ccdb0ee
to
930d80c
Compare
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- contracts/core/Dispatcher.sol (1 hunks)
- src/deploy.ts (1 hunks)
- src/evm/contracts/factories/Dispatcher__factory.ts (1 hunks)
- src/evm/schemas/tx.ts (1 hunks)
- src/tx.ts (1 hunks)
- src/utils/io.ts (2 hunks)
- test/upgradeableProxy/upgrades/DispatcherV2Initializable.sol (1 hunks)
Files not summarized due to errors (1)
- src/evm/contracts/factories/Dispatcher__factory.ts: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (5)
- contracts/core/Dispatcher.sol
- src/deploy.ts
- src/evm/schemas/tx.ts
- src/tx.ts
- src/utils/io.ts
Additional comments not posted (3)
test/upgradeableProxy/upgrades/DispatcherV2Initializable.sol (1)
Line range hint
33-38
: Verify the correct usage ofreinitializer(3)
.The change from
reinitializer(2)
toreinitializer(3)
allows for a new initialization version. Ensure that this change aligns with the contract's upgrade and initialization logic.src/evm/contracts/factories/Dispatcher__factory.ts (2)
2114-2114
: Verify the correct handling of transaction overrides.Ensure that the
overrides
parameter is correctly passed and handled in thegetDeployTransaction
method.Verification successful
Verified the correct handling of transaction overrides.
The
overrides
parameter is correctly passed and handled in thegetDeployTransaction
method within theDispatcher__factory.ts
file. The method usesoverrides || {}
to ensure that a default empty object is provided ifoverrides
is not supplied.
- File:
src/evm/contracts/factories/Dispatcher__factory.ts
- Lines: 2114-2114
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of transaction overrides in the codebase. # Test: Search for the usage of getDeployTransaction method. Expect: Correct handling of overrides parameter. rg --type typescript $'getDeployTransaction'Length of output: 78
Script:
#!/bin/bash # Description: Verify the correct handling of transaction overrides in the codebase. # Test: Search for the usage of getDeployTransaction method. Expect: Correct handling of overrides parameter. rg --type ts 'getDeployTransaction'Length of output: 5429
2114-2114
: Ensure the new placeholders in the bytecode are correct.The placeholders
__$d825222459c46c14afb2efe0967c30e98d$__
and__$f61eb90c6f674e787d51c07f105fa231e2$__
should match the actual addresses ofIbc
andIbcUtils
libraries respectively.
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.
LGTM small but good change - curious does this not change the bindings?
We didn't change the interface (making a function external vs public doesn't change the abi at all) so no go bindings were changed. We DID change the contract bytecode, which is why we have new typescript bindings |
PR to make dispatcher reinitializable
this adds a minor change to our dispatcher implementation, where we have our initializing function inherit from
reinitializer(2)
rather than initializer. This will allow us to override the init version for our existing prod + staging contracts to allow setting the fee vault.Correspondingly, there is a minor feature addition to add initialization to trasnactions as well (similar to existing functionality of constructor initialization), which renders $INITDATA sent to tx specs. This will be utilized in proxy upgrades which utilize initializers (i.e. any contract upgrades which need to modify setting state)
Summary by CodeRabbit
ethers
library.