feat: update factory address in wrapper contract #139
Conversation
WalkthroughThis pull request introduces updates to the ERC20 wrapper and factory contract deployment process. The changes include modifying the upgrade process in Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/upgrade.go (1)
52-58: Consider logging more details for troubleshooting.
When ignoring thevm.ErrContractAddressCollisionerror, it could be helpful to log or emit an event indicating the wrapper contract was already deployed. This will facilitate observability and debugging of upgrade steps.x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
32-37: Addemitevent insetFactory.
Updating the factory is a significant on-chain event. Emitting an event can help off-chain clients track or respond to this change.function setFactory(address newFactory) external onlyChain { require(newFactory != address(0), "invalid factory address"); factory = ERC20Factory(newFactory); + emit FactoryUpdated(newFactory); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/upgrade.go(4 hunks)x/evm/contracts/erc20_wrapper/ERC20Wrapper.go(2 hunks)x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol(2 hunks)x/evm/keeper/erc20.go(3 hunks)x/evm/keeper/genesis.go(1 hunks)x/evm/keeper/genesis_test.go(2 hunks)x/evm/types/expected_keeper.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run test and upload codecov
🔇 Additional comments (22)
app/upgrade.go (6)
25-25: Version naming looks good.
This version bump to 0.6.8 is consistent with the workflow. No issues here.
Line range hint
59-70: Deployment sequence verification.
After deploying the ERC20 factory at line 59, the subsequent logic to update existing ERC20 contracts in line 70 is conditional upon a successful deployment. This approach helps maintain consistent state. Just ensure that any partial state updates in preceding lines are rolled back or handled properly when an error occurs.
81-85: Function documentation is clear.
The docstring accurately describes the upgrade steps for ERC20 contracts. Great clarity on each bullet point.
86-92: Validate new code references.
Line 86 referenceserc20.Erc20MetaData.Bin. Confirm that this artifact is updated to the correct bytecode and tested on the latest deployment.
93-95: Hashing approach is standard.
Keccak256 is appropriate for obtaining the code hash. No issues.
96-127: Solid error handling and iteration over all contracts.
This loop robustly updates each contract’s code, code hash, and code size. Error handling looks good, returning from the iteration immediately if any step fails.x/evm/types/expected_keeper.go (1)
69-70: Adding ABI getters expands integration capabilities.
Exposing these two new methods (GetERC20FactoryABIandGetERC20WrapperABI) is beneficial for interacting with factory and wrapper contracts. Ensure thorough unit coverage for all new interface methods.x/evm/keeper/genesis_test.go (3)
6-6: Import addition is pertinent.
You are usingsdk.Contextwithin your test, so importing the SDK is necessary and correct.
108-141: Test_DeployERC20Factory provides solid coverage.
The test appropriately verifies that the factory address is updated both before and after wrapper deployment. Good job verifying outcomes withrequire.Equalandrequire.NotEqual.
143-159: Helper function clarifies reading from wrapper.
queryFactoryAddressFromWrapperis well-crafted to retrieve the current factory address. It improves test readability and reusability.x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
13-13: Inheritance extension from ERC20ACL.
By inheritingERC20ACL, you’ve centralized the contract’s access control logic. Ensure that the new ACL restrictions align with your security requirements.x/evm/keeper/genesis.go (3)
65-68: Ensure robust error handling.
Storing the factory address in the keeper and checking for errors is correct. The approach is appropriate for redeployment scenarios. No further adjustments needed here.
70-81: Validate chain call to set wrapper's factory.
This logic ensures that the wrapper contract references the correct factory address. The error checks before and after packing the ABI and executing the EVM call are comprehensive. If the wrapper contract hasn’t been set yet, the code safely skips the setFactory call. The added calls and checks appear solid.
83-83: Return statement.
Returning nil maintains a clean exit pathway without any additional side effects. Looks good.x/evm/keeper/erc20.go (6)
22-22: Added import for erc20_wrapper.
The new import references the wrapper contract definitions. It's consistent with the usage below.
33-33: New field in ERC20Keeper for ERC20WrapperABI.
Adding this ABI reference aligns with the introduced method calls. Make sure to confirm that it’s properly instantiated everywhere needed.
47-50: Load wrapper ABI gracefully.
Retrieving the ABI for the wrapper contract follows the same pattern as the existing code for the ERC20 and factory ABIs. The error handling is straightforward and appropriate.
57-57: Initialize ERC20Keeper with wrapper ABI.
Ensures the keeper has all relevant contract ABIs for future interactions. This is coherent with the usage pattern.
65-69: Expose GetERC20FactoryABI.
Providing the factory ABI for external usage is correct. Implementation is trivial and accurate.
70-74: Expose GetERC20WrapperABI.
Similarly, making the wrapper ABI accessible for other components is clean and consistent with the new code additions.x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (2)
34-35: Updated ABI and bytecode.
These changes reflect the newsetFactorymethod. The contract metadata is consistent with the introduction of the new function signature in the ABI.
371-390: New setFactory methods.
The three new transactor/session methods allow on-chain updates of the factory address. The naming and parameters follow best practices, and the additions align with the contract's new functionality. Error handling is delegated to the standard binding's pattern, which is standard for Go-Ethereum contract calls.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
- Coverage 57.03% 56.97% -0.06%
==========================================
Files 110 110
Lines 10049 10073 +24
==========================================
+ Hits 5731 5739 +8
- Misses 3495 3507 +12
- Partials 823 827 +4
|
Description
update the factory contract address in the wrapper contract whenever the factory contract is redeployed on the chain.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests