feat: apply audit results about token minting and distribution #336
Conversation
WalkthroughThis update introduces various enhancements and modifications across multiple files in the Kroma project. Key changes include updates to storage layout definitions and hexadecimal values, deployment of tokens and mint managers, adjustments to gas costs in tests, and additions of new ownership transfer mechanisms. The modifications ensure more accurate handling of recipients during token distribution and introduce new events for consistent state tracking. The assertion checks during deployment have been adjusted accordingly. Changes
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Could you update the scenario for deployments you wrote in confluence? |
I already updated it. |
seolaoh
force-pushed
the
feat/apply-token-audit
branch
from
a183cf7 to
438d3e6
Compare
seolaoh
force-pushed
the
feat/implement-vesting-wallet
branch
from
422d572 to
d9596bf
Compare
|
Should this PR be merged to |
seolaoh
force-pushed
the
feat/apply-token-audit
branch
from
438d3e6 to
027bb77
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
Files selected for processing (16)
- kroma-bindings/bindings/governancetoken.go (5 hunks)
- kroma-bindings/bindings/governancetoken_more.go (1 hunks)
- kroma-bindings/bindings/mintmanager.go (4 hunks)
- kroma-bindings/bindings/mintmanager_more.go (1 hunks)
- op-e2e/e2eutils/setup.go (1 hunks)
- packages/contracts/.gas-snapshot (3 hunks)
- packages/contracts/.storage-layout (1 hunks)
- packages/contracts/contracts/governance/GovernanceToken.sol (5 hunks)
- packages/contracts/contracts/governance/MintManager.sol (5 hunks)
- packages/contracts/contracts/test/GovernanceToken.t.sol (6 hunks)
- packages/contracts/contracts/test/KromaVestingWallet.t.sol (1 hunks)
- packages/contracts/contracts/test/MintManager.t.sol (4 hunks)
- packages/contracts/deploy/L1/018-L1MintManager.ts (1 hunks)
- packages/contracts/deploy/L1/019-L1GovernanceToken.ts (1 hunks)
- packages/contracts/deploy/L2/012-MintManager.ts (1 hunks)
- packages/contracts/deploy/L2/013-GovernanceToken.ts (1 hunks)
Files not summarized due to errors (3)
- kroma-bindings/bindings/governancetoken.go: Error: Message exceeds token limit
- kroma-bindings/bindings/governancetoken_more.go: Error: Message exceeds token limit
- kroma-bindings/bindings/mintmanager.go: Error: Message exceeds token limit
Additional context used
golangci-lint
op-e2e/e2eutils/setup.go
38-38: func
uint64ToBigis unused(unused)
Additional comments not posted (55)
packages/contracts/deploy/L1/018-L1MintManager.ts (1)
32-34: LGTM! Ensure thependingOwneris correctly set.The assertion to check the
pendingOwnervariable ensures the new ownership transfer mechanism is correctly implemented.packages/contracts/deploy/L2/012-MintManager.ts (1)
34-36: LGTM! Ensure thependingOwneris correctly set.The assertion to check the
pendingOwnervariable ensures the new ownership transfer mechanism is correctly implemented.packages/contracts/deploy/L2/013-GovernanceToken.ts (1)
43-45: LGTM! Ensure thependingOwneris correctly set.The assertion to check the
pendingOwnervariable ensures the new ownership transfer mechanism is correctly implemented.packages/contracts/deploy/L1/019-L1GovernanceToken.ts (1)
44-46: LGTM! Ensure thependingOwneris correctly set.The assertion to check the
pendingOwnervariable ensures the new ownership transfer mechanism is correctly implemented.packages/contracts/contracts/governance/GovernanceToken.sol (3)
37-38: LGTM!The update to use
__Ownable2Step_init()aligns with the new ownership management mechanism.
100-100: LGTM!Emitting the
Mintevent is a good practice for transparency and tracking.
111-111: LGTM!Emitting the
Burnevent is a good practice for transparency and tracking.packages/contracts/contracts/governance/MintManager.sol (3)
4-4: LGTM!The update to use
Ownable2Stepaligns with the new ownership management mechanism.
124-125: LGTM!The require statement ensures that the function cannot be called before tokens are minted, adding a layer of safety.
138-143: LGTM!The
acceptOwnershipOfTokenfunction aligns with the new ownership management mechanism.packages/contracts/contracts/test/KromaVestingWallet.t.sol (1)
52-53: LGTM!The updates ensure proper ownership transfer during setup.
packages/contracts/contracts/test/GovernanceToken.t.sol (6)
20-23: LGTM!Adding the
MintandBurnevents is a good practice for transparency and tracking.
40-44: LGTM!The updates ensure proper ownership transfer during setup.
60-61: LGTM!Including event expectations in tests ensures that the events are emitted correctly.
73-74: LGTM!Including event expectations in tests ensures that the events are emitted correctly.
102-103: LGTM!Including event expectations in tests ensures that the events are emitted correctly.
112-120: LGTM!Including event expectations in tests ensures that the events are emitted correctly and expected reverts are tested properly.
packages/contracts/contracts/test/MintManager.t.sol (9)
41-42: LGTM!The
setUpfunction correctly initializes theMintManagerand verifies the pending owner.
43-45: LGTM!The
acceptOwnershipfunction call and the subsequent owner verification are correct.
53-54: LGTM!The
pendingOwnerverification forgovernanceTokenis correct.
55-57: LGTM!The
acceptOwnershipOfTokenfunction call and the subsequent owner verification are correct.
74-94: LGTM!The
test_constructor_sameRecipient_succeedsfunction correctly tests the constructor with duplicate recipients and verifies the expected behavior.
198-199: LGTM!The
test_renounceOwnershipOfToken_succeedsfunction correctly callstest_mint_succeedsbefore renouncing ownership.
214-218: LGTM!The
test_renounceOwnershipOfToken_beforeMinted_revertsfunction correctly tests the expected revert behavior when renouncing ownership before minting.
220-241: LGTM!The
test_transferAndAcceptOwnershipOfToken_succeedsfunction correctly tests the transfer and acceptance of ownership and verifies the expected behavior.
250-254: LGTM!The
test_acceptOwnershipOfToken_fromNotOwner_revertsfunction correctly tests the expected revert behavior when a non-owner tries to accept ownership.kroma-bindings/bindings/mintmanager_more.go (2)
12-12: LGTM!The storage layout correctly includes the new
_pendingOwnerfield.
16-16: LGTM!The deployed binary data for
MintManageris updated correctly.op-e2e/e2eutils/setup.go (7)
266-266: LGTM!The deployment of
GovernanceTokenon L1 follows the updated process correctly.
272-284: LGTM!The deployment of
L1MintManagerfollows the updated process correctly, including the initialization of shares.
284-289: LGTM!The acceptance of ownership for
L1MintManagerbysysCfgOwneris correctly implemented.
290-301: LGTM!The proxy upgrade and initialization of
L1GovernanceTokenare correctly implemented.
301-304: LGTM!The acceptance of ownership for
L1GovernanceTokenbyL1MintManageris correctly implemented.
308-310: LGTM!The check to ensure
GovernanceTokenis not distributed yet is correctly implemented.
317-317: LGTM!The minting and distribution of
GovernanceTokenare correctly implemented.kroma-bindings/bindings/governancetoken_more.go (1)
12-15: LGTM! But verify the JSON structure.The added
_pendingOwnerfield at slot 113 looks correct.However, ensure that the JSON structure is correct and follows the expected format.
Verification successful
JSON Structure is Valid
The extracted JSON string from the Go file has been successfully validated without any errors.
GovernanceTokenStorageLayoutJSONcontains a valid JSON structure.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the JSON structure in the storage layout. # Test: Validate the JSON structure. Expect: No errors. jq empty kroma-bindings/bindings/governancetoken_more.goLength of output: 115
Script:
#!/bin/bash # Description: Extract the JSON string from the Go file and validate its structure. # Extract the JSON string from the Go file. json_string=$(sed -n 's/const GovernanceTokenStorageLayoutJSON = "\(.*\)"/\1/p' kroma-bindings/bindings/governancetoken_more.go) # Validate the extracted JSON string. echo "$json_string" | jq emptyLength of output: 12427
Script:
#!/bin/bash # Description: Extract the JSON string from the Go file, clean it, and validate its structure. # Extract the JSON string from the Go file. json_string=$(sed -n 's/const GovernanceTokenStorageLayoutJSON = "\(.*\)"/\1/p' kroma-bindings/bindings/governancetoken_more.go) # Clean the extracted JSON string by removing escape characters. cleaned_json_string=$(echo "$json_string" | sed 's/\\"/"/g') # Validate the cleaned JSON string. echo "$cleaned_json_string" | jq emptyLength of output: 23265
packages/contracts/.gas-snapshot (5)
173-174: Verify the gas usage fortest_release_tokenAfterFullyVested_succeedsandtest_release_token_succeeds.Ensure the gas usage increases are justified by the changes made in the contract.
93-93: Verify the gas usage fortest_approve_succeeds.Ensure the gas usage increase is justified by the changes made in the contract.
Verification successful
The gas usage for
test_approve_succeedsis justified.The increase to 147735 gas is due to the addition of
MintandBurnevents and the switch toOwnable2StepUpgradeablein theGovernanceTokencontract.
GovernanceToken.sol: Addition ofMintandBurnevents.GovernanceToken.sol: Use ofOwnable2StepUpgradeable.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the gas usage for `test_approve_succeeds`. # Test: Check the gas usage for `test_approve_succeeds`. Expect: 147735 gas. grep 'GovernanceToken_Test:test_approve_succeeds()' packages/contracts/.gas-snapshotLength of output: 145
Script:
#!/bin/bash # Description: Verify changes in the GovernanceToken contract that might affect gas usage. # Find the GovernanceToken contract file fd 'GovernanceToken.sol' # Check for the addition of Mint and Burn events and the use of Ownable2StepUpgradeable grep -E 'Mint|Burn|Ownable2StepUpgradeable' $(fd 'GovernanceToken.sol')Length of output: 646
286-301: Verify the gas usage forMintManagerTest.Ensure the gas usage increases are justified by the changes made in the contract, including the addition of
_pendingOwnerand the switch toOwnable2StepUpgradeable.Verification successful
Verified the gas usage for
MintManagerTest.The gas usage values for the specified tests match the provided output, indicating that the changes made in the contract are accurately reflected.
MintManagerTest:test_acceptOwnershipOfToken_fromNotOwner_reverts() (gas: 13015)MintManagerTest:test_constructor_invalidLengthArray_reverts() (gas: 162680)MintManagerTest:test_constructor_sameRecipient_succeeds() (gas: 1059218)MintManagerTest:test_constructor_succeeds() (gas: 132879)MintManagerTest:test_constructor_tooManyShares_reverts() (gas: 688804)MintManagerTest:test_constructor_zeroRecipient_reverts() (gas: 164769)MintManagerTest:test_constructor_zeroShares_reverts() (gas: 164399)MintManagerTest:test_distribute_fromNotOwner_reverts() (gas: 13055)MintManagerTest:test_distribute_succeeds() (gas: 558730)MintManagerTest:test_mint_alreadyMinted_reverts() (gas: 248847)MintManagerTest:test_mint_fromNotOwner_reverts() (gas: 12959)MintManagerTest:test_mint_succeeds() (gas: 246927)MintManagerTest:test_renounceOwnershipOfToken_beforeMinted_reverts() (gas: 15197)MintManagerTest:test_renounceOwnershipOfToken_fromNotOwner_reverts() (gas: 13014)MintManagerTest:test_renounceOwnershipOfToken_succeeds() (gas: 254991)MintManagerTest:test_transferAndAcceptOwnershipOfToken_succeeds() (gas: 1430287)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the gas usage for `MintManagerTest`. # Test: Check the gas usage for the specified tests. Expect: Correct gas values. grep -E 'MintManagerTest:test_acceptOwnershipOfToken_fromNotOwner_reverts|test_constructor_invalidLengthArray_reverts|test_constructor_sameRecipient_succeeds|test_constructor_succeeds|test_constructor_tooManyShares_reverts|test_constructor_zeroRecipient_reverts|test_constructor_zeroShares_reverts|test_distribute_fromNotOwner_reverts|test_distribute_succeeds|test_mint_alreadyMinted_reverts|test_mint_fromNotOwner_reverts|test_mint_succeeds|test_renounceOwnershipOfToken_beforeMinted_reverts|test_renounceOwnershipOfToken_fromNotOwner_reverts|test_renounceOwnershipOfToken_succeeds|test_transferAndAcceptOwnershipOfToken_succeeds|test_transferOwnershipOfToken_fromNotOwner_reverts' packages/contracts/.gas-snapshotLength of output: 2721
94-101: Verify the gas usage fortest_burn_fromBridge_succeedstotest_mint_fromOwner_succeeds.Ensure the gas usage increases are justified by the changes made in the contract, including the addition of
MintandBurnevents and the switch toOwnable2StepUpgradeable.Verification successful
The gas usage for
test_burn_fromBridge_succeedstotest_mint_fromOwner_succeedsis justified by the changes made in the contract, including the addition ofMintandBurnevents and the switch toOwnable2StepUpgradeable.
- The
MintandBurnevents were found in the relevant contract files.- The
Ownable2StepUpgradeablewas correctly imported and used in theGovernanceTokencontract.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the gas usage for `test_burn_fromBridge_succeeds` to `test_mint_fromOwner_succeeds`. # Test: Check the gas usage for the specified tests. Expect: Correct gas values. grep -E 'GovernanceToken_Test:test_burn_fromBridge_succeeds|test_burn_fromNotBridge_reverts|test_mint_fromBridge_succeeds|test_mint_fromNotMinter_reverts|test_mint_fromOwner_succeeds' packages/contracts/.gas-snapshotLength of output: 555
Script:
#!/bin/bash # Verify the presence of `Mint` and `Burn` events in the contracts echo "Checking for Mint event..." rg "event Mint" --glob "**/*.sol" echo "Checking for Burn event..." rg "event Burn" --glob "**/*.sol" # Verify the switch to `Ownable2StepUpgradeable` echo "Checking for Ownable2StepUpgradeable..." rg "Ownable2StepUpgradeable" --glob "**/*.sol"Length of output: 1796
102-103: Verify the gas usage fortest_transferFrom_succeedsandtest_transfer_succeeds.Ensure the gas usage increases are justified by the changes made in the contract.
Verification successful
The gas usage increases for
test_transferFrom_succeedsandtest_transfer_succeedsare justified.The changes in
GovernanceToken.solinclude the addition ofMintandBurnevents, modifications in themintandburnfunctions to emit these events, and updates in the ownership logic. These modifications naturally lead to increased gas consumption due to the additional operations and event logging.
GovernanceToken.sol: AddedMintandBurnevents, changed ownership logic toOwnable2StepUpgradeable.GovernanceToken.t.sol: Updated test cases to include event expectations and ownership acceptance.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the gas usage for `test_transferFrom_succeeds` and `test_transfer_succeeds`. # Test: Check the gas usage for the specified tests. Expect: Correct gas values. grep -E 'GovernanceToken_Test:test_transferFrom_succeeds|test_transfer_succeeds' packages/contracts/.gas-snapshotLength of output: 239
Script:
#!/bin/bash # Description: Fetch contract changes related to `test_transferFrom_succeeds` and `test_transfer_succeeds` to verify gas usage justification. # Find the contract files that might have affected the gas usage contract_files=$(git diff --name-only HEAD~1 HEAD | grep -E '\.sol$') # Display the contract changes for file in $contract_files; do echo "Changes in $file:" git diff HEAD~1 HEAD -- $file echo doneLength of output: 15802
packages/contracts/.storage-layout (2)
382-383: Addition of_pendingOwnerinGovernanceTokencontract.The addition of
_pendingOwnerreflects the switch toOwnable2StepUpgradeable.
389-395: Addition of_pendingOwnerinMintManagercontract.The addition of
_pendingOwnerreflects the switch toOwnable2StepUpgradeable.kroma-bindings/bindings/mintmanager.go (8)
34-35: Update ABI and Binary DataThe ABI and binary data have been updated to include the new
pendingOwnerfunctionality. Ensure that this ABI is correctly reflected in the contract and that all changes are consistent with the Solidity contract.
360-375: Add Function:PendingOwnerThe function
PendingOwnerretrieves thependingOwneraddress. This is consistent with the new ownership transfer mechanism. The implementation follows the pattern used for other getter functions.
377-389: Add Session Functions:PendingOwnerThe session functions
PendingOwnerforMintManagerSessionandMintManagerCallerSessionare correctly added for retrieving thependingOwneraddress. These functions are consistent with the existing pattern for session-based functions.
453-473: Add Function:AcceptOwnershipThe
AcceptOwnershipfunction allows the pending owner to accept ownership. This function is correctly implemented and follows the pattern used for other mutator functions.
474-493: Add Function:AcceptOwnershipOfTokenThe
AcceptOwnershipOfTokenfunction allows the pending owner to accept ownership of the token. This function is correctly implemented and follows the pattern used for other mutator functions.
621-714: Add Iterator and Filter forOwnershipTransferStartedEventThe iterator and filter for the
OwnershipTransferStartedevent have been correctly added. These additions are consistent with the existing event handling patterns and will help in monitoring ownership transfer events.
716-760: Add Watcher forOwnershipTransferStartedEventThe watcher for the
OwnershipTransferStartedevent has been correctly added. This addition is consistent with the existing event subscription patterns and will help in real-time monitoring of ownership transfer events.
762-772: Add Parser forOwnershipTransferStartedEventThe parser for the
OwnershipTransferStartedevent has been correctly added. This addition is consistent with the existing event parsing patterns and will help in extracting event data from logs.kroma-bindings/bindings/governancetoken.go (4)
808-837: Confirm the correctness and consistency of thePendingOwnerfunction.The
PendingOwnerfunction appears to be correctly implemented. Ensure it is consistent with other similar functions in the contract.
963-982: Confirm the correctness and consistency of theAcceptOwnershipfunction.The
AcceptOwnershipfunction appears to be correctly implemented. Ensure it is consistent with other similar functions in the contract.
2350-2369: Confirm the correctness and consistency of theFilterOwnershipTransferStartedfunction.The
FilterOwnershipTransferStartedfunction appears to be correctly implemented. Ensure it is consistent with other similar event filters in the contract.
2371-2415: Confirm the correctness and consistency of theWatchOwnershipTransferStartedfunction.The
WatchOwnershipTransferStartedfunction appears to be correctly implemented. Ensure it is consistent with other similar event watchers in the contract.
Description
Since we use
Ownable2Stepcontract to set the owner ofGovernanceTokenandMintManager, the new owner should accept the ownership. The purpose is to reduce human error, so I thought it is more appropriate to follow this step when deploying the contracts via deploy script. So the deploy step is changed like below.Deploy Process
L1GovernanceTokenProxycontractL1MintManagercontractmintManagerOwneris set aspendingOwnerofL1MintManagerL1GovernanceTokencontract, upgrade proxy, and initializeL1MintManageris set aspendingOwnerofL1GovernanceTokenmintManagerOwneris set as admin ofL1GovernanceTokenProxyL1MintManager.acceptOwnership(),L1MintManager.acceptOwnershipOfToken()bymintManagerOwner> confirm ownership transfer ofL1MintManager,L1GovernanceTokenL1GovernanceTokenProxyto zero address bymintManagerOwnerTest Transactions on Holesky
acceptOwnershipofL1MintManageracceptOwnershipofL1GovernanceToken