feat: implement KRO-based validator system #374

seolaoh · 2024-08-23T04:51:14Z

Description

Implemented the new KRO-based validator system, which introduces KRO tokenomics and delegation to the validator system. The detailed specification can be found here.

…ssion

…ger`

…ger` to chain-ops

…idatorPool

* feat(contracts): remove KGH share and modify KGH delegation func * feat: review applied and restore stuffs related to undelegation * feat: review applied * feat(contracts): implement 1-step undelegation of KRO * feat: review applied * feat: review applied * chore: update comment * feat: review applied * feat(contracts): implement getKroAssets view func * chore: update interface of undelegate func * chore: update comment of undelegate func

* feat(contracts): remove KGH share and modify KGH delegation func * feat: review applied and restore stuffs related to undelegation * feat: review applied * feat(contracts): implement 1-step undelegation of KRO * feat(contracts): add claim boost reward before delegation of KGH * feat(contracts): implement 1-step undelegation of KGH * feat(contracts): implement 1-step undelegation of KGH batch * feat: review applied * feat: review applied * chore: update comment * fix: remove duplicated arg for claim of boost reward * feat: review applied * feat: review applied * feat(contracts): implement getKroAssets view func * chore: remove duplicated index increment * feat: transfer claimed boost reward when delegate KGH * chore: update interface of undelegate func * chore: restore condition check for transfer of claimed reward

* feat(contracts): remove KRO in KGH * feat(contracts): apply PR reviews

* feat: add withdraw method for validator * feat: apply PR reviews * feat: review applied * feat: review applied * feat: review applied

* feat: remove dup checks in the process of register * feat: update subcommands for validator system v2 * feat: update README for validator system v2 * feat: apply PR reviews

* feat: move termination index from config to each component * feat(contracts): add function that returns total validator balance * feat: add check for bond amount at validator system v2 * feat: apply PR reviews * feat: apply PR reviews

* fix(contracts): avoid divide by zero of kgh num * test(contracts): modify `Colosseum` tests related to V2 * feat(contracts): apply PR reviews * test(contracts): use delegator of common test in `AssetManager` tests * test(contracts): apply PR reviews

* test: fix validator manager contract tests * feat: review applied * feat: review applied * feat: review applied * feat: move revertSlash test

* chore(contracts): update bindings and snapshot * feat(contracts): modify deploy scripts * refac(contracts): reduce local vars in `Colosseum` to reduce code size * chore(chain-ops): set `ValidatorPool` termination index to 10 in devnet

* feat(validator): assert output submission condition before submit tx * test(validator): modify e2e tests related to V2

* fix(validator): disable validator to be turned on without V2 * feat(validator): improve bond log * refac(contracts): remove input argument of `tryUnjail`

… of SC (#370) * fix(contracts): avoid overflow error during calculating output reward of SC * feat(contracts): add replaced output submitter to event * feat(contracts): check jail period expired when a validator withdraws * chore(contracts): update bindings and snapshots

* feat(contracts): remove the comment that implies bond amount change * feat(contracts): save deleted output and enhance check when dismiss challenge * feat(contracts): restrict contract address cannot call `registerValidator` * feat(contracts): remove meaningless unchecked block * feat(contracts): add sanity checks for `BalancedWeightTree` * chore(contracts): update bindings and snapshots * feat: apply contract interface changes to client code * feat(contracts): apply PR review

coderabbitai · 2024-08-23T04:54:08Z

Walkthrough

The changes encompass the addition and modification of several smart contract bindings related to the Ethereum Optimism project. Key updates include the introduction of new contracts, enhancements to existing contracts' functionalities, updates to storage layouts, and adjustments to the Application Binary Interface (ABI). New tests have been added to ensure the correctness of functionalities for the AssetManager, ValidatorManager, and ValidatorPool contracts, along with improvements in their operational logic.

Changes

Files	Change Summary
`kroma-bindings/bindings/*.go`	Introduced new bindings for AssetManager and ValidatorManager, updated storage layouts and bytecode.
`packages/contracts/contracts/L1/*.sol`	Added new ValidatorManager contract and modified ValidatorPool for termination handling and state management.
`packages/contracts/contracts/test/*.sol`	Created test contracts for AssetManager, ValidatorManager, and BalancedWeightTree; improved test coverage and functionality validation.
`packages/contracts/contracts/interfaces/IValidatorManager.sol`	Defined the `IValidatorManager` interface with necessary functions for validator management.
`packages/contracts/contracts/test/CommonTest.t.sol`	Enhanced testing framework with mock contracts for ERC20 and ERC721 interactions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ValidatorManager
    participant ValidatorPool
    participant AssetManager

    User->>ValidatorManager: Register Validator
    ValidatorManager-->>User: Confirmation

    User->>AssetManager: Deposit Assets
    AssetManager-->>User: Success

    User->>ValidatorPool: Terminate Pool
    ValidatorPool-->>User: Termination Confirmation

sequenceDiagram
    participant ValidatorManager
    participant Validator
    participant RewardSystem

    ValidatorManager->>Validator: Activate Validator
    Validator-->>ValidatorManager: Activation Success

    Validator->>RewardSystem: Claim Rewards
    RewardSystem-->>Validator: Rewards Credited

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

0xHansLee

LFG 🚀

coderabbitai

Actionable comments posted: 8

Outside diff range, codebase verification and nitpick comments (9)

kroma-validator/cmd/validator/flags.go (2)
19-23: Clarify usage for AddressFlag.

The AddressFlag and WithdrawAccountFlag both involve addresses, which might cause confusion. Ensure the usage descriptions are clear and distinct.

Consider updating the usage description for clarity:
 var AddressFlag = &cli.StringFlag{
-	Usage:    "Address to receive ETH",
+	Usage:    "Recipient address for ETH",
31-35: Clarify usage for WithdrawAccountFlag.

The usage of WithdrawAccountFlag is similar to AddressFlag, which might cause confusion. Ensure the usage descriptions are clear and distinct.

Consider updating the usage description for clarity:
 var WithdrawAccountFlag = &cli.StringFlag{
-	Usage:    "Address to withdraw deposited asset token",
+	Usage:    "Account address for asset token withdrawal",
kroma-validator/cmd/README.md (4)
29-29: Use "number" instead of "amount" for countable nouns.

Consider using "number" instead of "amount" for the tokens to deposit.
-  - `--amount [value]` - _(Required)_ The amount of tokens to deposit initially (in Wei).
+  - `--amount [value]` - _(Required)_ The number of tokens to deposit initially (in Wei).
Tools

LanguageTool

[uncategorized] ~29-~29: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...- --amount [value] - (Required) The amount of tokens to deposit initially (in Wei)...

(AMOUNTOF_TO_NUMBEROF)

63-63: Use "number" instead of "amount" for countable nouns.

Consider using "number" instead of "amount" for the tokens to deposit.
-  - `--amount [value]` - _(Required)_ The amount of tokens to deposit (in Wei).
+  - `--amount [value]` - _(Required)_ The number of tokens to deposit (in Wei).
Tools

LanguageTool

[uncategorized] ~63-~63: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...- --amount [value] - (Required) The amount of tokens to deposit (in Wei). ```ba...

(AMOUNTOF_TO_NUMBEROF)

69-69: Use "withdrawal" instead of "withdraw".

The word "withdraw" is not a noun. Consider using "withdrawal".
- Note that withdraw of the deposited asset and reward must be done with the withdraw account that was set during the
+ Note that withdrawal of the deposited asset and reward must be done with the withdrawal account that was set during the
Tools

LanguageTool

[grammar] ~69-~69: The word ‘withdraw’ is not a noun. Did you mean “withdrawal”?
Context: ... asset and reward must be done with the withdraw account that was set during the registr...

(PREPOSITION_VERB)

70-70: Strengthen wording by using "ensure".

Consider using "ensure" instead of "make sure" for stronger wording.
- Please make sure that you must keep the private key of the withdraw account safe, since it cannot be
+ Please ensure that you must keep the private key of the withdrawal account safe, since it cannot be
Tools

LanguageTool

[style] ~70-~70: Consider using a different verb to strengthen your wording.
Context: ...was set during the registration. Please make sure that you must keep the private key of t...

(MAKE_SURE_ENSURE)

[grammar] ~70-~70: The word ‘withdraw’ is not a noun. Did you mean “withdrawal”?
Context: ...at you must keep the private key of the withdraw account safe, since it cannot be modifi...

(PREPOSITION_VERB)
kroma-validator/metrics/metrics.go (1)

33-34: Update documentation for Metricer interface.

The Metricer interface now includes RecordUnbondedDepositAmount and RecordValidatorStatus. Ensure that any documentation or usage examples are updated to reflect these changes.

Consider adding comments to explain the purpose of these new methods.
op-e2e/actions/l2_challenger_test.go (1)
Line range hint 401-514: Function ChallengeInvalidProofFail logic is comprehensive.

The function effectively tests the invalid proof failure scenario, covering interactions and outcomes for different validator versions.

However, there's an unused variable slashingAmount in the V2 logic. Consider removing or using it.

Apply this diff to address the unused variable warning:
- valStatus, inJail, afterAsset, afterAssetBonded, slashingAmount := rt.fetchValidatorStatus(rt.validator)
+ valStatus, inJail, afterAsset, afterAssetBonded, _ := rt.fetchValidatorStatus(rt.validator)
kroma-bindings/bindings/l2outputoracle.go (1)
Inconsistent function signatures detected for DeployL2OutputOracle.

The codebase contains two different signatures for the DeployL2OutputOracle function. Ensure that all instances are updated to use the new signature with the _validatorManager parameter.

Files with outdated signature:

op-bindings/bindings/l2outputoracle.go

Files with updated signature:

kroma-bindings/bindings/l2outputoracle.go

kroma-validator/abi_test.go

Please update the outdated instances to maintain consistency.

Analysis chain

Line range hint 47-56: Verify the usage of the updated DeployL2OutputOracle function.

The function signature has been updated to include a new parameter _validatorManager. Ensure that all calls to this function in the codebase have been updated accordingly.

Run the following script to verify the function usage:
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `DeployL2OutputOracle` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'DeployL2OutputOracle'
Length of output: 1919

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bc896c5 and 75ae796.

Files ignored due to path filters (8)

kroma-bindings/artifacts.json is excluded by !**/*.json
kroma-chain-ops/genesis/testdata/allocs-l1.json is excluded by !**/*.json
kroma-chain-ops/genesis/testdata/deploy.json is excluded by !**/*.json
kroma-chain-ops/genesis/testdata/l1-deployments.json is excluded by !**/*.json
kroma-chain-ops/genesis/testdata/test-deploy-config-devnet-l1.json is excluded by !**/*.json
kroma-chain-ops/genesis/testdata/test-deploy-config-full.json is excluded by !**/*.json
ops-devnet/docker-compose.yml is excluded by !**/*.yml
packages/contracts/deploy-config/devnetL1-template.json is excluded by !**/*.json

Files selected for processing (65)

kroma-bindings/bindings/assetmanager_more.go (1 hunks)
kroma-bindings/bindings/colosseum_more.go (1 hunks)
kroma-bindings/bindings/l2outputoracle.go (9 hunks)
kroma-bindings/bindings/l2outputoracle_more.go (1 hunks)
kroma-bindings/bindings/validatormanager_more.go (1 hunks)
kroma-bindings/bindings/validatorpool.go (5 hunks)
kroma-bindings/bindings/validatorpool_more.go (1 hunks)
kroma-chain-ops/genesis/config.go (7 hunks)
kroma-chain-ops/genesis/config_test.go (2 hunks)
kroma-chain-ops/genesis/layer_one.go (5 hunks)
kroma-chain-ops/genesis/layer_one_test.go (2 hunks)
kroma-devnet/devnet/init.py (1 hunks)
kroma-validator/abi_test.go (1 hunks)
kroma-validator/challenger.go (18 hunks)
kroma-validator/cmd/README.md (1 hunks)
kroma-validator/cmd/main.go (2 hunks)
kroma-validator/cmd/validator/cmd.go (1 hunks)
kroma-validator/cmd/validator/flags.go (1 hunks)
kroma-validator/config.go (11 hunks)
kroma-validator/event.go (2 hunks)
kroma-validator/flags/flags.go (5 hunks)
kroma-validator/guardian.go (16 hunks)
kroma-validator/l2_output_submitter.go (15 hunks)
kroma-validator/metrics/metrics.go (6 hunks)
kroma-validator/metrics/noop.go (1 hunks)
kroma-validator/status.go (1 hunks)
kroma-validator/validator.go (4 hunks)
op-e2e/actions/l1_miner.go (1 hunks)
op-e2e/actions/l2_challenger.go (1 hunks)
op-e2e/actions/l2_challenger_test.go (16 hunks)
op-e2e/actions/l2_runtime.go (7 hunks)
op-e2e/actions/l2_validator.go (5 hunks)
op-e2e/actions/l2_validator_test.go (3 hunks)
op-e2e/actions/user_test.go (4 hunks)
op-e2e/bridge_test.go (1 hunks)
op-e2e/config/init.go (2 hunks)
op-e2e/e2eutils/geth/wait.go (2 hunks)
op-e2e/e2eutils/secrets.go (9 hunks)
op-e2e/e2eutils/setup.go (7 hunks)
op-e2e/e2eutils/setup_test.go (1 hunks)
op-e2e/e2eutils/validator/validator.go (1 hunks)
op-e2e/e2eutils/wait/transactions.go (1 hunks)
op-e2e/setup.go (19 hunks)
op-e2e/system_test.go (8 hunks)
packages/contracts/.gas-snapshot (4 hunks)
packages/contracts/.storage-layout (3 hunks)
packages/contracts/contracts/L1/AssetManager.sol (1 hunks)
packages/contracts/contracts/L1/Colosseum.sol (34 hunks)
packages/contracts/contracts/L1/L2OutputOracle.sol (17 hunks)
packages/contracts/contracts/L1/ValidatorManager.sol (1 hunks)
packages/contracts/contracts/L1/ValidatorPool.sol (9 hunks)
packages/contracts/contracts/L1/ZKMerkleTrie.sol (2 hunks)
packages/contracts/contracts/L1/interfaces/IAssetManager.sol (1 hunks)
packages/contracts/contracts/L1/interfaces/IValidatorManager.sol (1 hunks)
packages/contracts/contracts/libraries/Atan2.sol (1 hunks)
packages/contracts/contracts/libraries/BalancedWeightTree.sol (1 hunks)
packages/contracts/contracts/libraries/Uint128Math.sol (1 hunks)
packages/contracts/contracts/test/AssetManager.t.sol (1 hunks)
packages/contracts/contracts/test/BalancedWeightTree.t.sol (1 hunks)
packages/contracts/contracts/test/BenchmarkTest.t.sol (3 hunks)
packages/contracts/contracts/test/Colosseum.t.sol (40 hunks)
packages/contracts/contracts/test/CommonTest.t.sol (13 hunks)
packages/contracts/contracts/test/KromaPortal.t.sol (3 hunks)
packages/contracts/contracts/test/L2OutputOracle.t.sol (20 hunks)
packages/contracts/contracts/test/ValidatorManager.t.sol (1 hunks)

Files skipped from review due to trivial changes (5)

kroma-bindings/bindings/assetmanager_more.go
kroma-bindings/bindings/validatormanager_more.go
kroma-validator/status.go
op-e2e/e2eutils/geth/wait.go
packages/contracts/contracts/L1/ZKMerkleTrie.sol

Additional context used

LanguageTool

kroma-validator/cmd/README.md

[uncategorized] ~29-~29: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...- --amount [value] - (Required) The amount of tokens to deposit initially (in Wei)...

(AMOUNTOF_TO_NUMBEROF)

[uncategorized] ~63-~63: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...- --amount [value] - (Required) The amount of tokens to deposit (in Wei). ```ba...

(AMOUNTOF_TO_NUMBEROF)

[grammar] ~69-~69: The word ‘withdraw’ is not a noun. Did you mean “withdrawal”?
Context: ... asset and reward must be done with the withdraw account that was set during the registr...

(PREPOSITION_VERB)

[style] ~70-~70: Consider using a different verb to strengthen your wording.
Context: ...was set during the registration. Please make sure that you must keep the private key of t...

(MAKE_SURE_ENSURE)

[grammar] ~70-~70: The word ‘withdraw’ is not a noun. Did you mean “withdrawal”?
Context: ...at you must keep the private key of the withdraw account safe, since it cannot be modifi...

(PREPOSITION_VERB)

golangci-lint

op-e2e/actions/l2_validator.go

188-188: ineffectual assignment to err

(ineffassign)

op-e2e/actions/l2_runtime.go

115-115: func (*Runtime).setupMaliciousChallenger1 is unused

(unused)

119-119: func (*Runtime).setupMaliciousChallenger2 is unused

(unused)

op-e2e/actions/l2_challenger_test.go

509-509: SA4006: this value of slashingAmount is never used

(staticcheck)

op-e2e/system_test.go

1781-1781: ineffectual assignment to err

(ineffassign)

Additional comments not posted (473)

kroma-validator/metrics/noop.go (2)
25-25: LGTM! Verify the usage of RecordValidatorStatus.

The addition of RecordValidatorStatus enhances the metrics functionality.

Ensure that the new method is correctly integrated and utilized in the codebase.

Run the following script to verify the method usage:

Verification successful

The RecordValidatorStatus method is correctly integrated and utilized.

The method is implemented in both noopMetrics and Metrics, indicating it is part of a common interface or pattern. It is used in l2_output_submitter.go and challenger.go, showing proper integration into the codebase.

Files and Lines:

kroma-validator/l2_output_submitter.go: Usage of RecordValidatorStatus.

kroma-validator/challenger.go: Usage of RecordValidatorStatus.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all method calls to `RecordValidatorStatus`.

# Test: Search for the method usage. Expect: Correct integration and utilization.
rg --type go -A 5 $'RecordValidatorStatus'
Length of output: 2129
24-25: LGTM! Verify the impact of renaming RecordDepositAmount.

The renaming of RecordDepositAmount to RecordUnbondedDepositAmount is consistent with the new focus on unbonded deposits.

Ensure that all calls to RecordDepositAmount have been updated to the new method name.

Run the following script to verify the method usage:

Verification successful

Renaming Verified Successfully
All references to RecordDepositAmount have been updated to RecordUnbondedDepositAmount, confirming consistent renaming across the codebase.

No occurrences of RecordDepositAmount were found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all method calls to `RecordUnbondedDepositAmount`.

# Test: Search for the method usage. Expect: Only occurrences of the new method name.
rg --type go -A 5 $'RecordUnbondedDepositAmount'
Length of output: 2359

Script:
#!/bin/bash
# Description: Verify that there are no remaining references to the old method name `RecordDepositAmount`.

# Test: Search for the old method name. Expect: No occurrences of the old method name.
rg --type go -A 5 $'RecordDepositAmount'
Length of output: 39
op-e2e/e2eutils/setup_test.go (1)

32-35: LGTM! Improved type consistency in balance comparisons.

The changes enhance the precision of balance comparisons by ensuring type consistency with Uint64().

kroma-validator/event.go (2)

19-28: LGTM! The function changes improve type safety and validation.

The function correctly validates the log topic length and constructs the bindings.ColosseumChallengeCreated object. The error message is clear, and the conversion operations are safe.

31-43: LGTM! The function changes improve type safety and validation.

The function correctly validates the log topic length and constructs the bindings.L2OutputOracleOutputSubmitted object. The error message is clear, and the conversion operations are safe.

packages/contracts/contracts/libraries/Uint128Math.sol (1)

15-33: LGTM! The mulDiv function is well-implemented with overflow checks.

The function uses inline assembly for multiplication and effectively checks for overflow with the require statement. The logic is correct and follows best practices for safe arithmetic operations.

op-e2e/e2eutils/wait/transactions.go (1)

15-51: LGTM! The function handles L2 transactions with proper context management and error handling.

The function correctly retrieves the nonce, signs the transaction, and verifies the receipt. Context management and error handling are robust, and transaction parameters are appropriately set.

kroma-validator/abi_test.go (1)

36-36: Verify the impact of the additional address parameter.

The addition of common.Address{0xcc} as a parameter might affect the configuration of the L2OutputOracle. Ensure that this change aligns with the intended behavior and update any relevant documentation or tests if necessary.

packages/contracts/contracts/test/BalancedWeightTree.t.sol (1)

9-71: LGTM! The test functions cover key operations.

The test functions for insert, update, remove, and select operations are well-structured and utilize fuzz testing effectively. Ensure the tests are run in various environments to confirm their robustness.

packages/contracts/contracts/libraries/Atan2.sol (1)

1-69: Verify the precision and range of the atan2 calculation.

The implementation uses a polynomial approximation with fixed-point arithmetic. Ensure that the precision (1E-12) and the range of inputs are suitable for all intended use cases. Consider adding tests to validate the accuracy of the results.

op-e2e/actions/l2_validator_test.go (2)

41-52: LGTM!

The RunValidatorPoolTest function is well-structured and correctly implements the test logic for the validator pool.

54-95: LGTM!

The RunValidatorManagerTest function is comprehensive and covers the necessary logic for testing the validator manager.

kroma-validator/cmd/README.md (1)

73-105: LGTM!

The section on Validator System V1 commands clearly indicates their deprecated status and provides alternative guidance.

kroma-validator/cmd/main.go (9)

34-41: LGTM!

The "register" command is correctly defined with necessary flags.

44-47: LGTM!

The "activate" command is correctly defined without additional flags.

49-52: LGTM!

The "unjail" command is correctly defined without additional flags.

54-66: LGTM!

The "changeCommission" command and its subcommands are correctly defined.

71-75: LGTM!

The "depositKro" command is correctly defined with necessary flags.

77-82: LGTM!

The "deposit" command is clearly marked as deprecated with alternative guidance provided.

84-89: LGTM!

The "withdraw" command is clearly marked as deprecated with alternative guidance provided.

91-96: LGTM!

The "withdrawTo" command is clearly marked as deprecated with alternative guidance provided.

98-101: LGTM!

The "unbond" command is clearly marked as deprecated with alternative guidance provided.

op-e2e/actions/l2_challenger.go (1)

33-35: Ensure the logic change aligns with business requirements.

The logic now checks if the challenger can create a challenge using CanCreateChallenge. Ensure this aligns with the intended business logic and requirements, as it changes the conditions under which challenges can be initiated.

Consider verifying that this change does not inadvertently allow or prevent challenges under unintended conditions.

kroma-validator/metrics/metrics.go (2)

50-51: Ensure consistency in metric naming and usage.

The UnbondedDepositAmount and ValidatorStatus fields have been added to the Metrics struct. Verify that these metrics are consistently named and used throughout the codebase.

Consider checking other parts of the codebase to ensure these metrics are correctly integrated.

145-152: Review implementation of new metric recording methods.

The methods RecordUnbondedDepositAmount and RecordValidatorStatus have been added. Ensure that these methods are correctly implemented and that they align with the intended metrics tracking strategy.

Consider testing these methods to ensure they accurately record the intended metrics.

op-e2e/e2eutils/validator/validator.go (6)

38-61: LGTM!

The NewHelper function is well-structured and correctly initializes the Helper struct with contract bindings and error handling.

63-73: LGTM!

The DepositToValPool function correctly handles transaction creation and receipt waiting with proper error handling.

75-86: LGTM!

The UnbondValPool function effectively handles transaction creation and status checking with appropriate error handling.

88-97: LGTM!

The ApproveAssetToken function correctly manages transaction approval and receipt waiting with appropriate error handling.

99-114: LGTM!

The RegisterToValMgr function effectively manages the registration process with appropriate error handling and transaction management.

116-131: LGTM!

The Delegate function correctly manages the delegation process with appropriate error handling and transaction management.
op-e2e/config/init.go (2)
13-19: LGTM!

The import cleanup enhances code readability by consolidating related imports.

130-130: LGTM! But verify the usage of the new configuration parameter.

The addition of ValidatorManagerRoundDurationSeconds is logical and aligns with existing parameters.

Run the following script to verify the usage of ValidatorManagerRoundDurationSeconds in the codebase:

Verification successful

Usage of ValidatorManagerRoundDurationSeconds is verified and correct.

The parameter is consistently assigned and validated across the codebase, ensuring its proper usage.

Files and lines where it's used:

op-e2e/config/init.go

op-e2e/setup.go

op-e2e/e2eutils/setup.go

kroma-chain-ops/genesis/config.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of `ValidatorManagerRoundDurationSeconds` in the codebase.

# Test: Search for the usage of the new configuration parameter. Expect: Occurrences where it's utilized.
rg --type go 'ValidatorManagerRoundDurationSeconds'
Length of output: 1292
kroma-chain-ops/genesis/layer_one_test.go (2)

15-20: LGTM!

The import reorganization enhances code clarity and consistency.

64-66: LGTM!

The new test logic for validating the VALIDATORMANAGER address enhances test coverage and robustness.

kroma-chain-ops/genesis/config_test.go (1)

117-120: LGTM! The additional assertions enhance test coverage.

The added checks for non-zero addresses in TestL1Deployments ensure that the new deployment components are correctly initialized.

kroma-validator/flags/flags.go (3)

61-66: LGTM! The ValMgrAddressFlag is correctly implemented.

The flag allows specifying the ValidatorManager contract address and is marked as required.

67-72: LGTM! The AssetManagerAddressFlag is correctly implemented.

The flag allows specifying the AssetManager contract address and is marked as required.

131-135: LGTM! The GuardianPollIntervalFlag is correctly implemented.

The flag specifies the polling interval for guardian inspections, with a default value of one minute.

kroma-validator/validator.go (9)

111-116: LGTM! The conditional creation of the challenger is a good optimization.

This change ensures that the challenger is only instantiated when necessary, optimizing resource usage.

150-153: LGTM! The nil check for TxManager in the Start method is a good addition.

This check prevents runtime errors if TxManager is not initialized.

156-159: LGTM! The nil check for l2os in the Start method is a good addition.

This check ensures that the l2os component is only started if it is initialized.

162-165: LGTM! The nil check for challenger in the Start method is a good addition.

This check ensures that the challenger component is only started if it is initialized.

168-171: LGTM! The nil check for guardian in the Start method is a good addition.

This check ensures that the guardian component is only started if it is initialized.

180-183: LGTM! The nil check for TxManager in the Stop method is a good addition.

This check prevents runtime errors if TxManager is not initialized.

186-189: LGTM! The nil check for l2os in the Stop method is a good addition.

This check ensures that the l2os component is only stopped if it is initialized.

192-195: LGTM! The nil check for challenger in the Stop method is a good addition.

This check ensures that the challenger component is only stopped if it is initialized.

198-201: LGTM! The nil check for guardian in the Stop method is a good addition.

This check ensures that the guardian component is only stopped if it is initialized.

op-e2e/e2eutils/secrets.go (8)

22-34: Ensure consistency in mnemonic paths.

The addition of the Guardian mnemonic path is consistent with the existing structure. Verify that the mnemonic paths align with the intended key management strategy.

60-60: Confirm the addition of the Guardian field.

The Guardian field has been added to the MnemonicConfig structure. Ensure that this addition aligns with the intended design and is used appropriately throughout the codebase.

Line range hint 111-125: Check error handling for Guardian key retrieval.

The retrieval of the Guardian private key follows the same pattern as other keys. Ensure that error handling is consistent and that the Guardian key is correctly utilized.

142-142: Guardian key integration in Secrets structure.

The Guardian key is now part of the Secrets structure. Confirm that this integration is consistent with the overall key management approach.

169-169: Guardian key addition in Addresses structure.

The Guardian address is included in the Addresses structure. Verify that this addition is correctly reflected in all relevant parts of the codebase.

202-202: Guardian address computation.

The Guardian address is computed using the public key. Ensure that this computation is accurate and consistent with other address computations.

226-226: Guardian address in Addresses structure.

The Guardian address is now part of the Addresses structure. Confirm that this addition aligns with the intended design and is used appropriately throughout the codebase.

245-245: Guardian address inclusion in All method.

The Guardian address is included in the All method. Ensure that this inclusion is consistent with other addresses and that the method functions as expected.

packages/contracts/contracts/test/BenchmarkTest.t.sol (3)

13-13: Formatting change in setPrevBaseFee function.

The setPrevBaseFee function has been reformatted for improved readability. Ensure that functionality remains unchanged.

74-74: Logic change in setUp method of GasBenchMark_KromaPortal.

The logic for setting the L2 timestamp has been updated. Verify that this change aligns with the intended logic and does not introduce errors.

223-223: Parameter removal in warpToSubmitTime function.

The warpToSubmitTime function no longer requires a parameter. Ensure that this change is consistent with the function's internal logic and state management.

kroma-chain-ops/genesis/layer_one.go (3)

8-8: Import statement adjustments.

Ensure that the removal of duplicate imports and the inclusion of necessary dependencies are correct.

104-104: PostProcessL1DeveloperGenesis function enhancement.

The PostProcessL1DeveloperGenesis function has been enhanced to handle additional operations. Verify that the new logic is correctly implemented and does not introduce errors.

197-219: Setting governance token balances on L1.

The logic for setting governance token balances has been added. Ensure that this logic is correct and aligns with the intended functionality.

op-e2e/actions/l1_miner.go (3)

232-235: Refactor approved for ActEmptyBlock.

The refactoring of ActEmptyBlock to call ActL1StartBlock and ActL1EndBlock improves readability and modularity.

237-243: New method includeL1BlockBySender approved.

The addition of includeL1BlockBySender enhances granularity in block inclusion operations by handling transactions based on the sender's address.

244-248: New method includeL1BlockByTx approved.

The addition of includeL1BlockByTx enhances granularity in block inclusion operations by handling transactions based on the transaction hash.

op-e2e/actions/l2_validator.go (12)

30-37: Enhancements to ValidatorCfg approved.

The addition of ValidatorManagerAddr and AssetManagerAddr fields enhances the configuration options for validators.

41-53: Enhancements to L2Validator approved.

The addition of valMgrContractAddr and assetManagerContractAddr fields improves the internal state management of the L2Validator.

131-144: New method CalculateWaitTime approved.

The CalculateWaitTime method is well-structured and ensures that necessary conditions are met before calculating the wait time.

146-160: New method ActSubmitL2Output approved.

The ActSubmitL2Output method correctly handles transaction data preparation and submission, ensuring non-blocking behavior.

162-170: New method ActDeposit approved.

The ActDeposit method correctly handles ABI packing and transaction sending for deposits to the validator pool.

172-185: New method ActRegisterValidator approved.

The ActRegisterValidator method correctly handles ABI packing and transaction sending for validator registration.

187-199: New method ActApprove approved.

The ActApprove method correctly handles ABI packing and transaction sending for approvals to the asset manager.

Tools

golangci-lint

188-188: ineffectual assignment to err

(ineffassign)

201-206: New method fetchOutput approved.

The fetchOutput method correctly handles output fetching and error checking for a given block number.

208-213: New method isValPoolTerminated approved.

The isValPoolTerminated method correctly handles the termination check using the output index.

215-220: New method getValidatorStatus approved.

The getValidatorStatus method correctly handles status retrieval and error checking for a validator.

222-227: New method isInJail approved.

The isInJail method correctly handles the jail status check and error checking for a validator.

Line range hint 228-258: Method sendTx approved.

The sendTx method is well-structured, handling gas estimation, transaction signing, and sending asynchronously.

Tools

golangci-lint

188-188: ineffectual assignment to err

(ineffassign)

kroma-validator/config.go (4)

Line range hint 34-50: Enhancements to Config approved.

The addition of ValidatorManagerAddr, AssetManagerAddr, and GuardianPollInterval fields enhances the configurability of the validator.

Line range hint 87-119: Enhancements to CLIConfig approved.

The addition of ValMgrAddress, AssetManagerAddress, and GuardianPollInterval fields ensures enhanced configurability through the command-line interface.

Line range hint 161-176: Updates to NewConfig approved.

The NewConfig function correctly parses and initializes new fields from the command-line context, ensuring proper configuration setup.

Line range hint 210-267: Updates to NewValidatorConfig approved.

The NewValidatorConfig function correctly validates and parses new address fields, ensuring robust configuration handling.
op-e2e/actions/user_test.go (3)
134-134: Ensure consistency with ValidatorManagerTrustedValidator.

The addition of the equality check for ValidatorManagerTrustedValidator enhances validation. Ensure that this change is consistent with other parts of the codebase where similar checks are performed.

Run the following script to verify consistency:

Verification successful

Verified Consistency of ValidatorManagerTrustedValidator Usage

The usage of ValidatorManagerTrustedValidator is consistent across the codebase, with similar equality checks in various test and setup files. No inconsistencies were found.

op-e2e/setup.go

op-e2e/e2eutils/setup.go

kroma-chain-ops/genesis/config.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify consistency of `ValidatorManagerTrustedValidator` usage.

# Test: Search for all occurrences of `ValidatorManagerTrustedValidator`. Expect: Consistent usage.
rg --type go 'ValidatorManagerTrustedValidator'
Length of output: 1056
141-148: Review additions to ValidatorCfg.

The addition of ValidatorManagerAddr and AssetManagerAddr to ValidatorCfg suggests an expanded validation setup. Ensure these fields are correctly utilized in the codebase.

Run the following script to verify usage:

Verification successful

Fields ValidatorManagerAddr and AssetManagerAddr are correctly utilized.

The fields are integrated into the codebase and used in various contexts, such as configuration setup and contract interactions, indicating their correct utilization.

Files involved:

op-e2e/setup.go

op-e2e/actions/user_test.go

op-e2e/actions/l2_runtime.go

op-e2e/actions/l2_validator.go

kroma-validator/l2_output_submitter.go

kroma-validator/challenger.go

kroma-validator/config.go

kroma-validator/flags/flags.go

kroma-validator/cmd/validator/cmd.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify usage of `ValidatorManagerAddr` and `AssetManagerAddr`.

# Test: Search for all occurrences of `ValidatorManagerAddr` and `AssetManagerAddr`. Expect: Correct utilization.
rg --type go 'ValidatorManagerAddr|AssetManagerAddr'
Length of output: 3056
Line range hint 263-274: Review method change to includeL1BlockBySender.

The method for including L1 blocks has been changed to includeL1BlockBySender. Ensure that this change is reflected consistently across the codebase.

Run the following script to verify consistency:

Verification successful

Consistent Usage of includeL1BlockBySender Verified: The method includeL1BlockBySender is consistently used across the codebase, with no discrepancies found in its usage. The method appears in multiple files, and all instances reflect the same naming and parameter conventions.

Files with occurrences:

op-e2e/actions/user_test.go

op-e2e/actions/l2_challenger_test.go

op-e2e/actions/l1_miner.go

op-e2e/actions/l2_runtime.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify consistency of `includeL1BlockBySender` usage.

# Test: Search for all occurrences of `includeL1BlockBySender`. Expect: Consistent usage.
rg --type go 'includeL1BlockBySender'
Length of output: 2872
op-e2e/e2eutils/setup.go (4)
82-82: Ensure consistency with ValidatorManagerTrustedValidator.

The addition of the equality check for ValidatorManagerTrustedValidator enhances validation. Ensure that this change is consistent with other parts of the codebase where similar checks are performed.

133-148: Review allocation logic refinement.

The changes ensure that only the balance is updated when an address already exists in the allocation. This enhances allocation integrity. Ensure this logic is correctly implemented.

271-315: Review new function RedeployValPoolToTerminate.

The function facilitates redeployment of the ValidatorPool with an updated termination index, enhancing flexibility. Ensure the implementation aligns with expected transaction handling practices.

122-122: Review addition of ValidatorManagerRoundDurationSeconds.

The addition of ValidatorManagerRoundDurationSeconds suggests a separation of concerns in managing validator operations. Ensure this field is correctly utilized in the codebase.

Run the following script to verify usage:

Verification successful

Verified Usage of ValidatorManagerRoundDurationSeconds.

The ValidatorManagerRoundDurationSeconds field is correctly utilized across the codebase. It is involved in configuration setup and validation, ensuring its value is consistent with other configuration parameters.

Files and Usages:

op-e2e/setup.go: Sets the value.

op-e2e/e2eutils/setup.go: Sets the value.

op-e2e/config/init.go: Sets the value.

kroma-chain-ops/genesis/config.go: Contains validation logic to ensure its correctness.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify usage of `ValidatorManagerRoundDurationSeconds`.

# Test: Search for all occurrences of `ValidatorManagerRoundDurationSeconds`. Expect: Correct utilization.
rg --type go 'ValidatorManagerRoundDurationSeconds'
Length of output: 1292
kroma-validator/cmd/validator/cmd.go (13)

22-69: Review function Register.

The function registers a validator by sending a transaction with specific parameters. Ensure that all error handling and transaction processes are correctly implemented.

72-97: Review function Activate.

The function activates a validator by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

100-125: Review function Unjail.

The function attempts to unjail a validator by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

128-155: Review function InitCommissionChange.

The function initializes a commission change by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

158-183: Review function FinalizeCommissionChange.

The function finalizes a commission change by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

186-222: Review function DepositKro.

The function deposits KRO by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

225-252: Review function Deposit.

The function deposits an amount by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

255-287: Review function Withdraw.

The function withdraws an amount by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

290-324: Review function WithdrawTo.

The function withdraws an amount to a specific address by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

327-352: Review function Unbond.

The function unbonds by sending a transaction. Ensure that all error handling and transaction processes are correctly implemented.

355-381: Review helper function approve.

The function approves a transaction by sending it to the asset manager. Ensure that all error handling and transaction processes are correctly implemented.

383-402: Review helper function sendTransaction.

The function sends a transaction using the transaction manager. Ensure that all error handling and transaction processes are correctly implemented.

404-411: Review helper function newTxManager.

The function creates a new transaction manager. Ensure that all error handling and transaction processes are correctly implemented.
op-e2e/actions/l2_runtime.go (9)
103-104: Refactoring approved for setupMaliciousValidator.

The refactoring to use the setupValidator helper function improves code reuse and clarity.

Line range hint 159-183: Enhancements approved for bindContracts.

The additions of new contract bindings for ValidatorManager and AssetManager enhance the system's capability to interact with these components.

366-376: Enhancements approved for fetchValidatorStatus.

The additional return values provide more detailed information about the validator's status, enhancing the function's utility.

379-380: Improvements approved for includeL1BlockBySender.

The changes improve the clarity and maintainability of the code.

383-384: Improvements approved for includeL1BlockByTx.

The changes improve the clarity and maintainability of the code.

68-71: Verify the usage of the new deltaTimeOffset parameter.

The deltaTimeOffset parameter has been added to the defaultRuntime function. Ensure that all calls to this function are updated to pass the correct value for this parameter.

Run the following script to verify the function usage:

Verification successful

The deltaTimeOffset parameter is correctly used in all instances of defaultRuntime.

All calls to the defaultRuntime function have been updated to include the new deltaTimeOffset parameter, ensuring consistent usage across the codebase.

op-e2e/actions/l2_validator_test.go

op-e2e/actions/l2_challenger_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `defaultRuntime` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'defaultRuntime'
Length of output: 4313
99-100: Verify the usage of the new setInvalidBlockNumber parameter.

The setInvalidBlockNumber parameter has been added to the setupHonestValidator function. Ensure that all calls to this function are updated to pass the correct value for this parameter.

Run the following script to verify the function usage:

Verification successful

Verified: The setInvalidBlockNumber parameter is correctly used.

The setupHonestValidator function calls in the codebase have been updated to include the new setInvalidBlockNumber parameter, ensuring consistency with the modified function signature.

op-e2e/actions/l2_validator_test.go: Calls to setupHonestValidator correctly pass the setInvalidBlockNumber parameter.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `setupHonestValidator` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'setupHonestValidator'
Length of output: 1136
213-234: Verify the usage of the new version parameter.

The version parameter has been added to the setupOutputSubmitted function. Ensure that all calls to this function are updated to pass the correct value for this parameter.

Run the following script to verify the function usage:

Verification successful

Verified: The version parameter is correctly used in all calls to setupOutputSubmitted.

The function calls in the test files l2_challenger_test.go and l2_validator_test.go have been updated to include the version parameter, ensuring consistency with the new function signature. No issues found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `setupOutputSubmitted` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'setupOutputSubmitted'
Length of output: 3903
238-239: Verify the usage of the new version parameter.

The version parameter has been added to the setupChallenge function. Ensure that all calls to this function are updated to pass the correct value for this parameter.

Run the following script to verify the function usage:

Verification successful

The version parameter is correctly used in the setupChallenge function calls.

All calls to the setupChallenge function in the op-e2e/actions/l2_challenger_test.go file have been updated to include the new version parameter, indicating that the codebase is consistent with the updated function signature.

The setupChallenge function is called with the version parameter in multiple instances within the test file.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `setupChallenge` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'setupChallenge'
Length of output: 3932
packages/contracts/contracts/libraries/BalancedWeightTree.sol (6)

58-116: Implementation approved for insert function.

The logic for inserting nodes into the tree is correct, and the use of unchecked blocks is appropriate for this context.

128-166: Implementation approved for update function.

The function correctly handles weight updates and adjusts the tree structure accordingly. The use of unchecked blocks is appropriate.

177-202: Implementation approved for remove function.

The function correctly handles node removal and updates the tree structure. The use of unchecked blocks is appropriate.

214-242: Implementation approved for select function.

The function correctly performs a weighted selection among the nodes. The use of unchecked blocks is appropriate.

252-277: Implementation approved for _promote function.

The function correctly handles node promotion to optimize tree traversal. The use of unchecked blocks is appropriate.

287-339: Implementation approved for _demote function.

The function correctly handles node demotion to optimize tree traversal. The use of unchecked blocks is appropriate.

kroma-devnet/devnet/__init__.py (3)

275-312: Enhancements approved for devnet_deploy function.

The enhancements improve the deployment process by integrating additional components and enhancing interaction with the validator management system.

Line range hint 163-193: Implementation approved for deploy_contracts function.

The function correctly handles contract deployment, including the necessary setup for the create2 deployer.

Line range hint 195-204: Implementation approved for init_devnet_l1_deploy_config function.

The function correctly sets up the deployment configuration, including optional timestamp updates.

packages/contracts/contracts/L1/L2OutputOracle.sol (7)

9-9: Import statement for IValidatorManager is correct.

The addition of the import statement is necessary for the changes in the contract.

25-28: Addition of VALIDATOR_MANAGER variable is appropriate.

The VALIDATOR_MANAGER variable is introduced to enhance validator management functionality.

91-98: Enhancement to OutputReplaced event is beneficial.

The addition of the newSubmitter parameter improves tracking of output submissions.

Line range hint 110-135: Constructor update is valid and necessary.

The addition of the IValidatorManager parameter ensures proper initialization of the contract.

Line range hint 218-283: Conditional logic in submitL2Output function is well-implemented.

The function now adapts to the state of the validator pool, enhancing flexibility in output submission.

286-306: Addition of setNextFinalizeOutputIndex function is appropriate.

The function enhances control over the finalization process with proper access control.

426-435: Addition of nextOutputMinL2Timestamp function is beneficial.

The function simplifies the computation of valid timestamps for output submissions.

kroma-bindings/bindings/l2outputoracle_more.go (2)

12-12: Updates to L2OutputOracleStorageLayoutJSON are correct.

The addition of nextFinalizeOutputIndex and updated type references ensure alignment with the contract's storage structure.

16-16: Update to L2OutputOracleDeployedBin is appropriate.

The updated binary representation aligns with the contract's logic and storage structure.

packages/contracts/contracts/L1/interfaces/IAssetManager.sol (8)

22-29: Definition of Asset struct is well-structured.

The struct includes necessary fields for managing validator assets effectively.

38-40: Definition of KroDelegator struct is appropriate.

The struct includes necessary fields for managing KRO delegation.

52-56: Definition of KghDelegator struct is well-structured.

The struct includes necessary fields for managing KGH delegation.

69-75: Definition of Vault struct is comprehensive.

The struct includes necessary fields for managing a validator's vault effectively.

77-192: Event definitions are well-defined.

The events provide necessary tracking for actions related to KRO and KGH delegation and withdrawal.

203-240: Error definitions are appropriate.

The errors provide necessary handling for common issues related to asset management.

242-403: Function definitions for asset information retrieval are well-structured.

The functions provide necessary access to various asset-related information.

405-478: Function definitions for asset management actions are comprehensive.

The functions provide necessary actions for managing assets effectively.

packages/contracts/contracts/L1/interfaces/IValidatorManager.sol (33)

1-2: Ensure compatibility with Solidity version.

The pragma directive specifies Solidity version 0.8.15. Ensure that this version is compatible with the rest of the codebase and any dependencies.

12-38: Enum ValidatorStatus is well-defined.

The ValidatorStatus enum provides clear and distinct statuses for validators. Ensure that these statuses are used consistently throughout the implementation.

40-73: Struct ConstructorParams is well-documented.

The ConstructorParams struct is well-documented, providing clarity on the purpose of each field. Ensure that these parameters are correctly utilized in the contract implementation.

75-91: Struct Validator is well-documented.

The Validator struct is well-documented, providing clarity on the purpose of each field. Ensure that these fields are correctly utilized in the contract implementation.

93-122: Events are well-defined.

The events ValidatorRegistered, ValidatorActivated, and ValidatorStopped are well-defined and provide useful information for tracking validator actions. Ensure that these events are emitted correctly in the contract implementation.

124-163: Events for commission changes are well-defined.

The events ValidatorCommissionChangeInitiated and ValidatorCommissionChangeFinalized are well-defined and provide useful information for tracking commission changes. Ensure that these events are emitted correctly in the contract implementation.

165-203: Events for rewards and slashing are well-defined.

The events RewardDistributed, ChallengeRewardDistributed, Slashed, and SlashReverted are well-defined and provide useful information for tracking rewards and slashing actions. Ensure that these events are emitted correctly in the contract implementation.

213-261: Errors are well-defined.

The errors NotAllowedCaller, InvalidConstructorParams, ImproperValidatorStatus, InsufficientAsset, MaxCommissionRateExceeded, SameCommissionRate, NotInitiatedCommissionChange, NotElapsedCommissionChangeDelay, NotElapsedJailPeriod, and NotSelectedPriorityValidator are well-defined and provide clear error messages. Ensure that these errors are used correctly in the contract implementation.

263-276: Function registerValidator is well-documented.

The registerValidator function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

278-282: Function activateValidator is well-documented.

The activateValidator function is well-documented, providing clarity on its purpose. Ensure that this function is implemented correctly in the contract.

284-291: Function tryActivateValidator is well-documented.

The tryActivateValidator function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

293-300: Function afterSubmitL2Output is well-documented.

The afterSubmitL2Output function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

302-308: Function initCommissionChange is well-documented.

The initCommissionChange function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

310-315: Function finalizeCommissionChange is well-documented.

The finalizeCommissionChange function is well-documented, providing clarity on its purpose. Ensure that this function is implemented correctly in the contract.

317-321: Function tryUnjail is well-documented.

The tryUnjail function is well-documented, providing clarity on its purpose. Ensure that this function is implemented correctly in the contract.

323-329: Function bondValidatorKro is well-documented.

The bondValidatorKro function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

331-337: Function unbondValidatorKro is well-documented.

The unbondValidatorKro function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

339-350: Function slash is well-documented.

The slash function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

352-358: Function revertSlash is well-documented.

The revertSlash function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

360-366: Function updateValidatorTree is well-documented.

The updateValidatorTree function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

368-375: Function noSubmissionCount is well-documented.

The noSubmissionCount function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

377-384: Function getCommissionRate is well-documented.

The getCommissionRate function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

386-393: Function getPendingCommissionRate is well-documented.

The getPendingCommissionRate function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

395-402: Function canFinalizeCommissionChangeAt is well-documented.

The canFinalizeCommissionChangeAt function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

404-411: Function checkSubmissionEligibility is well-documented.

The checkSubmissionEligibility function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

413-419: Function nextValidator is well-documented.

The nextValidator function is well-documented, providing clarity on its purpose and return value. Ensure that this function is implemented correctly in the contract.

421-428: Function getStatus is well-documented.

The getStatus function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

430-437: Function inJail is well-documented.

The inJail function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

439-446: Function jailExpiresAt is well-documented.

The jailExpiresAt function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

448-455: Function isActive is well-documented.

The isActive function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

457-466: Function getWeight is well-documented.

The getWeight function is well-documented, providing clarity on its purpose and parameters. Ensure that this function is implemented correctly in the contract.

468-473: Function activatedValidatorCount is well-documented.

The activatedValidatorCount function is well-documented, providing clarity on its purpose and return value. Ensure that this function is implemented correctly in the contract.

475-480: Function activatedValidatorTotalWeight is well-documented.

The activatedValidatorTotalWeight function is well-documented, providing clarity on its purpose and return value. Ensure that this function is implemented correctly in the contract.

packages/contracts/contracts/test/L2OutputOracle.t.sol (18)

9-9: LGTM! Import statement for IValidatorManager is correct.

The import statement is necessary for the new functionality related to validator management.

20-23: LGTM! Streamlined setUp function.

The changes improve the setup process by removing redundant lines and focusing on essential tasks.

28-29: LGTM! Added check for validatorManager in constructor test.

The addition ensures that the constructor initializes all necessary components.

42-43: LGTM! Updated constructor parameters in test_constructor_badTimestamp_reverts.

The inclusion of the _validatorManager parameter is necessary for the updated constructor.

56-57: LGTM! Updated constructor parameters in test_constructor_l2BlockTimeZero_reverts.

The inclusion of the _validatorManager parameter is necessary for the updated constructor.

70-71: LGTM! Updated constructor parameters in test_constructor_submissionInterval_reverts.

The inclusion of the _validatorManager parameter is necessary for the updated constructor.

89-90: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

99-100: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

116-117: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

129-130: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

142-143: LGTM! Simplified warpToSubmitTime calls.

The simplification improves readability.

Also applies to: 148-149, 154-155, 160-161

219-224: LGTM! Added test for nextOutputMinL2Timestamp.

The test ensures accurate timestamp calculations for the next output.

236-237: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

266-267: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

279-280: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

290-291: LGTM! Simplified warpToSubmitTime call.

The simplification improves readability.

425-485: LGTM! Added L2OutputOracle_ValidatorSystemUpgrade_Test contract.

The tests ensure correct interactions with the ValidatorPool and ValidatorManager.

Line range hint 519-563: LGTM! Added L2OutputOracleUpgradeable_Test contract.

The tests ensure that the upgrade process is secure and that state variables are correctly initialized.

kroma-bindings/bindings/validatorpool_more.go (1)

16-16: Verify the new binary string for ValidatorPoolDeployedBin.

The binary string has been completely replaced, indicating significant changes to the contract. Ensure that this new binary aligns with the intended functionality and does not introduce any unforeseen issues.

Consider running tests or simulations to validate the new binary's behavior and interactions with other components.

kroma-validator/guardian.go (12)

47-51: Verify initialization and usage of new fields.

The challengeCreatedSub and challengeCreatedChan fields have been added to manage challenge creation events. Ensure they are correctly initialized and used throughout the code.

142-145: Verify goroutine management for scanPrevChallenges.

The scanPrevChallenges method is now run in a goroutine. Ensure that this goroutine is correctly managed and terminated to prevent resource leaks.

154-163: Verify resource management in Stop method.

The Stop method now unsubscribes challengeCreatedSub and closes challengeCreatedChan. Ensure that all resources are correctly released to prevent memory leaks.

187-193: Verify initialization logic for challengeCreatedChan and challengeCreatedSub.

The initSub method initializes the challengeCreatedChan and challengeCreatedSub. Ensure that the initialization logic is correct and handles errors appropriately.

196-250: Verify logic for scanning and processing challenges.

The scanPrevChallenges method scans for previous challenges and processes them. Ensure that the logic is correct and efficient, and handles edge cases appropriately.

279-286: Verify consistency of headL1 usage.

The inspectorLoop method now uses headL1 instead of currentL1. Ensure that this change is consistent with the rest of the code and aligns with the intended logic.

407-409: Verify event handling logic for challengeCreatedChan.

The subscriptionLoop method now handles events from challengeCreatedChan. Ensure that the event handling logic is correct and efficient.

464-505: Verify logic for tracking challenges and sending timeout transactions.

The processChallengerTimeout method tracks created challenges and sends timeout transactions. Ensure that the logic is correct and handles errors appropriately.

616-639: Verify logic for checking guardian period.

The isInGuardianPeriod method checks if an output is within the guardian period. Ensure that the logic is correct and efficient, and handles edge cases appropriately.

641-666: Verify transaction logic for challenger timeout.

The tryChallengerTimeoutTx method attempts to send a challenger timeout transaction. Ensure that the logic is correct and handles errors appropriately.

735-739: Verify transaction crafting logic for challenger timeout.

The challengerTimeout method crafts a challenger timeout transaction. Ensure that the logic is correct and handles errors appropriately.

769-773: Verify logic for retrieving challenge status.

The getChallengeStatus method retrieves the status of a challenge. Ensure that the logic is correct and efficient, and handles errors appropriately.

packages/contracts/contracts/L1/ValidatorManager.sol (20)

1-184: Contract setup and constructor logic look good.

The use of immutable variables for addresses and the parameter validation in the constructor are appropriate.

189-211: Function registerValidator logic is sound.

The function correctly checks for caller validity, asset sufficiency, and handles activation.

216-221: Function activateValidator logic is correct.

The function ensures that only ready and non-jailed validators are activated.

226-229: Function tryActivateValidator logic is appropriate.

The function is correctly restricted to the AssetManager and checks the validator's readiness.

234-249: Function afterSubmitL2Output logic is well-implemented.

The function handles reward distribution, bonding, and priority validator updates effectively.

254-268: Function initCommissionChange logic is sound.

The function correctly validates the status and commission rate before initiating the change.

273-290: Function finalizeCommissionChange logic is correct.

The function ensures the delay period has elapsed and updates the commission rate accordingly.

295-307: Function tryUnjail logic is appropriate.

The function correctly handles the unjailing process and reactivation of validators.

312-314: Function bondValidatorKro logic is sound.

The function is correctly restricted to the Colosseum and bonds the validator's KRO.

319-321: Function unbondValidatorKro logic is appropriate.

The function is correctly restricted to the Colosseum and unbonds the validator's KRO.

326-344: Function slash logic is well-implemented.

The function effectively handles slashing and challenge reward distribution.

350-366: Function revertSlash logic is correct.

The function appropriately reverts slashing and unjails the validator.

371-379: Function checkSubmissionEligibility logic is sound.

The function correctly checks the validator's selection and activity status.

384-386: Function getCommissionRate logic is appropriate.

The function correctly retrieves the commission rate from the validator's information.

391-393: Function getPendingCommissionRate logic is correct.

The function appropriately retrieves the pending commission rate from the validator's information.

398-400: Function activatedValidatorCount logic is sound.

The function correctly calculates the count of activated validators.

405-407: Function getWeight logic is appropriate.

The function correctly retrieves the weight of a validator from the tree's node map.

412-414: Function jailExpiresAt logic is correct.

The function appropriately retrieves the jail expiration timestamp for a validator.

419-426: Function updateValidatorTree logic is sound.

The function correctly updates the validator tree based on the validator's status.

431-445: Function nextValidator logic is appropriate.

The function correctly determines the next priority validator for output submission.

op-e2e/actions/l2_challenger_test.go (7)

7-62: Test setup and helper functions are well-structured.

The setup allows for flexible testing of different scenarios and validator versions.

Line range hint 64-149: Function ChallengeBasic logic is comprehensive.

The function effectively tests the basic challenge scenario, covering interactions and outcomes for different validator versions.

Line range hint 152-232: Function ChallengeAsserterBisectTimeout logic is sound.

The function effectively tests the asserter bisect timeout scenario, covering interactions and outcomes for different validator versions.

Line range hint 235-313: Function ChallengeChallengerBisectTimeout logic is appropriate.

The function effectively tests the challenger bisect timeout scenario, covering interactions and outcomes for different validator versions.

Line range hint 316-398: Function ChallengeChallengerProvingTimeout logic is sound.

The function effectively tests the challenger proving timeout scenario, covering interactions and outcomes for different validator versions.

Line range hint 517-607: Function ChallengeForceDeleteOutputBySecurityCouncil logic is appropriate.

The function effectively tests the force delete output scenario by the security council, covering interactions and outcomes for different validator versions.

Tools

golangci-lint

509-509: SA4006: this value of slashingAmount is never used

(staticcheck)

Line range hint 611-726: Function MultipleChallenges logic is comprehensive.

The function effectively tests multiple challenges scenario, covering interactions and outcomes for different validator versions.

packages/contracts/contracts/L1/AssetManager.sol (49)

4-12: Imports look good.

The imported interfaces and libraries are appropriate for the contract's functionalities.

26-76: State variables are well-defined.

The constants, immutables, and mappings align with the contract's purpose.

79-103: Modifiers are well-structured.

The modifiers provide necessary access control and are appropriately defined.

111-138: Constructor is well-implemented.

The constructor parameters and assignments are appropriate for initializing the contract.

143-148: Function getKroTotalShareBalance is correctly implemented.

The function retrieves the total KRO share balance for a delegator as expected.

153-155: Function getKroAssets is correctly implemented.

The function converts shares to KRO assets using an internal conversion function.

160-162: Function getKghNum is correctly implemented.

The function retrieves the number of KGH tokens for a delegator as expected.

167-169: Function previewDelegate is correctly implemented.

The function previews the number of shares for a given amount of assets using an internal conversion function.

174-176: Function previewUndelegate is correctly implemented.

The function previews the amount of assets for a given number of shares using an internal conversion function.

181-186: Function canUndelegateKroAt is correctly implemented.

The function calculates when a delegator can undelegate KRO based on the last delegation time and the minimum delegation period.

191-199: Function canUndelegateKghAt is correctly implemented.

The function calculates when a delegator can undelegate KGH based on the token's delegation time and the minimum delegation period.

204-206: Function canWithdrawAt is correctly implemented.

The function calculates when a validator can withdraw assets based on the last deposit time and the minimum delegation period.

211-220: Function getKghReward is correctly implemented.

The function calculates the KGH reward for a delegator based on stored reward data.

225-227: Function getWithdrawAccount is correctly implemented.

The function retrieves the withdraw account for a validator as expected.

232-234: Function totalKroAssets is correctly implemented.

The function retrieves the total KRO assets for a validator as expected.

239-241: Function totalKghNum is correctly implemented.

The function retrieves the total number of KGH tokens for a validator as expected.

246-248: Function totalValidatorKro is correctly implemented.

The function retrieves the total KRO for a validator as expected.

253-255: Function totalValidatorKroBonded is correctly implemented.

The function retrieves the total bonded KRO for a validator as expected.

260-262: Function totalValidatorKroNotBonded is correctly implemented.

The function retrieves the total non-bonded KRO for a validator by subtracting bonded KRO from total KRO.

271-273: Function reflectiveWeight is correctly implemented.

The function calculates the reflective weight of a validator as the sum of total KRO and validator KRO.

284-294: Function depositToRegister is correctly implemented.

The function allows the ValidatorManager to deposit KRO for registration, checks for a zero address, and updates the vault before emitting an event.

299-308: Function deposit is correctly implemented.

The function allows validators to deposit KRO, checks for zero assets and validator status, updates the vault, and emits an event.

313-328: Function withdraw is correctly implemented.

The function allows withdrawal of KRO by the withdraw account, checks for conditions, updates the vault, and transfers assets before emitting an event.

333-346: Function delegate is correctly implemented.

The function allows delegation of KRO to a validator, checks for conditions, transfers assets, updates the vault, and emits an event.

351-363: Function delegateKgh is correctly implemented.

The function allows delegation of a KGH token to a validator, claims rewards, transfers the token, updates the vault, and emits an event.

368-394: Function delegateKghBatch is correctly implemented.

The function allows batch delegation of KGH tokens to a validator, checks for conditions, claims rewards, transfers tokens, updates the vault, and emits an event.

399-415: Function undelegate is correctly implemented.

The function allows undelegation of KRO from a validator, checks for conditions, updates the vault, transfers assets, and emits an event.

420-442: Function undelegateKgh is correctly implemented.

The function allows undelegation of a KGH token from a validator, checks for conditions, claims rewards, updates the vault, transfers tokens, and emits an event.

447-485: Function undelegateKghBatch is correctly implemented.

The function allows batch undelegation of KGH tokens from a validator, checks for conditions, updates the vault, transfers tokens, and emits an event.

490-497: Function claimKghReward is correctly implemented.

The function allows claiming of KGH rewards, checks for conditions, transfers rewards, and emits an event.

505-515: Function bondValidatorKro is correctly implemented.

The function bonds KRO for a validator, checks for conditions, updates the vault, and emits an event.

523-535: Function unbondValidatorKro is correctly implemented.

The function unbonds KRO for a validator, updates the vault, and emits an event.

546-581: Function increaseBalanceWithReward is correctly implemented.

The function increases the balance with rewards for a validator, transfers rewards, updates the vault, and emits an event.

594-614: Function increaseBalanceWithChallenge is correctly implemented.

The function increases the balance with challenge rewards for a winner, calculates tax, transfers rewards, updates the vault, and returns the reward.

625-636: Function decreaseBalanceWithChallenge is correctly implemented.

The function decreases the balance with challenge penalties for a loser, updates the vault, and returns the penalty amount.

646-657: Function revertDecreaseBalanceWithChallenge is correctly implemented.

The function reverts the decrease in balance due to challenge penalties, updates the vault, and returns the refunded amount.

666-668: Function _totalKroShares is correctly implemented.

The function retrieves the total KRO shares for a validator as expected.

676-685: Function _convertToKroShares is correctly implemented.

The function converts assets to KRO shares using a mathematical operation.

693-702: Function _convertToKroAssets is correctly implemented.

The function converts shares to KRO assets using a mathematical operation.

711-723: Function _deposit is correctly implemented.

The function handles the deposit of KRO by a validator, transfers assets, updates the vault, and optionally updates the validator tree.

731-738: Function _withdraw is correctly implemented.

The function handles the withdrawal of KRO by a validator, checks for conditions, and updates the vault.

748-762: Function _delegate is correctly implemented.

The function handles the delegation of KRO to a validator, updating the vault with the delegated assets and shares.

771-781: Function _delegateKgh is correctly implemented.

The function handles the delegation of a KGH token to a validator, updating the vault and the delegator's token data.

790-800: Function _delegateKghBatch is correctly implemented.

The function handles the batch delegation of KGH tokens to a validator, updating the vault and the delegator's token data for multiple tokens.

810-822: Function _undelegate is correctly implemented.

The function handles the undelegation of KRO from a validator, updating the vault with the undelegated assets and shares.

832-844: Function _undelegateKgh is correctly implemented.

The function handles the undelegation of a KGH token from a validator, updating the vault and the delegator's token data.

853-862: Function _undelegateKghBatch is correctly implemented.

The function handles the batch undelegation of KGH tokens from a validator, updating the vault and the delegator's token data for multiple tokens.

873-884: Function _claimBoostedReward is correctly implemented.

The function calculates and claims the boosted reward for a delegator, updating the delegator's reward data.

889-896: Function onERC721Received is correctly implemented.

The function implements the ERC721Receiver interface and returns the correct selector.

kroma-validator/challenger.go (8)

13-14: Imports look good.

The imports are appropriate for the functionality described in the file.

46-60: Struct field changes are appropriate.

The changes enhance the struct's capability to manage interactions with additional contracts and improve naming consistency.

Line range hint 82-121: NewChallenger function changes are well-implemented.

The initialization of new fields and error handling improvements are appropriate.

Line range hint 131-194: InitConfig method updates are sound.

The logic for fetching and setting contract-related configurations is appropriate.

Line range hint 321-369: scanPrevOutputs method improvements are robust.

The refined error handling for event creation enhances the method's robustness.

648-701: CanCreateChallenge method changes ensure eligibility.

The additional checks for validator status and deposit amounts are appropriate.

Line range hint 580-599: handleChallenge method logic is comprehensive.

The logic for handling challenges based on their status and role is well-implemented.

Line range hint 822-828: OutputsAtIndex method logic is appropriate.

The logic for fetching outputs at a given index is straightforward and well-implemented.

packages/contracts/contracts/test/AssetManager.t.sol (47)

15-42: LGTM! Mock contract implementation is appropriate.

The MockAssetManager correctly extends the AssetManager and provides additional functionality for testing purposes.

45-54: LGTM! Mock contract implementation is appropriate.

The MockValidatorManager correctly extends the ValidatorManager and provides additional functionality for testing purposes.

64-105: LGTM! Test setup is correctly implemented.

The setUp function initializes the necessary mock contracts and sets up the environment for the tests.

107-112: LGTM! Output root submission is correctly simulated.

The _submitOutputRoot function correctly simulates the submission of an output root by a validator.

114-119: LGTM! Validator registration is correctly simulated.

The _registerValidator function correctly simulates the registration of a validator with the specified amount.

121-126: LGTM! Delegation process is correctly simulated.

The _delegate function correctly simulates the delegation of a specified amount to a validator.

128-134: LGTM! KGH delegation process is correctly simulated.

The _delegateKgh function correctly simulates the delegation of a KGH token to a validator.

136-147: LGTM! Batch KGH delegation process is correctly simulated.

The _delegateKghBatch function correctly simulates the batch delegation of KGH tokens to a validator.

149-156: LGTM! Undelegation process is correctly simulated.

The _undelegate function correctly simulates the undelegation of a specified amount from a validator.

158-162: LGTM! KGH undelegation process is correctly simulated.

The _undelegateKgh function correctly simulates the undelegation of a KGH token from a validator.

164-173: LGTM! Batch KGH undelegation process is correctly simulated.

The _undelegateKghBatch function correctly simulates the batch undelegation of KGH tokens from a validator.

175-183: LGTM! Constructor test is correctly implemented.

The test_constructor_succeeds function correctly verifies that the AssetManager constructor initializes the contract state as expected.

185-198: LGTM! Deposit to register test is correctly implemented.

The test_depositToRegister_succeeds function correctly verifies the deposit process and state changes for registering a validator.

200-203: LGTM! Caller access control test is correctly implemented.

The test_depositToRegister_callerNotValMgr_reverts function correctly verifies that the deposit reverts if the caller is not the Validator Manager.

205-209: LGTM! Zero withdrawal account test is correctly implemented.

The test_depositToRegister_zeroWithdrawAcc_reverts function correctly verifies that the deposit reverts if the withdrawal account is zero.

211-226: LGTM! Deposit test is correctly implemented.

The test_deposit_succeeds function correctly verifies the deposit process and state changes.

228-237: LGTM! Deposit activation test is correctly implemented.

The test_deposit_activate_succeeds function correctly verifies that the validator is activated when the deposit threshold is met.

239-248: LGTM! Non-activation deposit test is correctly implemented.

The test_deposit_notActivate_succeeds function correctly verifies that the validator remains inactive when the deposit is below the activation threshold.

250-262: LGTM! Jailed validator deposit test is correctly implemented.

The test_deposit_inJailNotActivate_succeeds function correctly verifies that a validator in jail is not activated upon deposit.

264-267: LGTM! Zero asset deposit test is correctly implemented.

The test_deposit_zeroAsset_reverts function correctly verifies that a deposit of zero assets reverts.

269-273: LGTM! Validator status condition test is correctly implemented.

The test_deposit_validatorStatusNone_reverts function correctly verifies that a deposit reverts if the validator status is none.

275-289: LGTM! Withdrawal test is correctly implemented.

The test_withdraw_succeeds function correctly verifies the withdrawal process and state changes.

291-297: LGTM! Withdrawal access control test is correctly implemented.

The test_withdraw_notWithdrawAcc_reverts function correctly verifies that a withdrawal reverts if the caller is not the withdrawal account.

299-305: LGTM! Zero asset withdrawal test is correctly implemented.

The test_withdraw_zeroAsset_reverts function correctly verifies that a withdrawal of zero assets reverts.

307-313: LGTM! Delegation period condition test is correctly implemented.

The test_withdraw_notElapsedMinDelegationPeriod_reverts function correctly verifies that a withdrawal reverts if the minimum delegation period has not elapsed.

315-327: LGTM! Jail period condition test is correctly implemented.

The test_withdraw_notExpiredJailPeriod_reverts function correctly verifies that a withdrawal reverts if the jail period has not expired.

330-337: LGTM! Asset sufficiency condition test is correctly implemented.

The test_withdraw_insufficientValidatorKro_reverts function correctly verifies that a withdrawal reverts if the validator's KRO is insufficient.

339-357: LGTM! Bonded KRO condition test is correctly implemented.

The test_withdraw_validatorKroBonded_reverts function correctly verifies that a withdrawal reverts if the validator's KRO is bonded.

359-376: LGTM! Delegation test is correctly implemented.

The test_delegate_succeeds function correctly verifies the delegation process and state changes.

378-381: LGTM! Validator status condition test is correctly implemented.

The test_delegate_validatorStatusNone_reverts function correctly verifies that delegation reverts if the validator status is none.

383-389: LGTM! Exited status condition test is correctly implemented.

The test_delegate_validatorStatusExited_reverts function correctly verifies that delegation reverts if the validator status is exited.

391-398: LGTM! Jail condition test is correctly implemented.

The test_delegate_validatorInJail_reverts function correctly verifies that delegation reverts if the validator is in jail.

400-406: LGTM! Zero asset delegation test is correctly implemented.

The test_delegate_zeroAsset_reverts function correctly verifies that delegation of zero assets reverts.

408-424: LGTM! KGH delegation test is correctly implemented.

The test_delegateKgh_succeeds function correctly verifies the delegation process and state changes for a KGH token.

426-441: LGTM! Boosted reward claim test is correctly implemented.

The test_delegateKgh_claimBoostedReward_succeeds function correctly verifies the claim of boosted rewards after KGH delegation.

443-446: LGTM! Validator status condition test is correctly implemented.

The test_delegateKgh_validatorStatusNone_reverts function correctly verifies that KGH delegation reverts if the validator status is none.

448-454: LGTM! Exited status condition test is correctly implemented.

The test_delegateKgh_validatorStatusExited_reverts function correctly verifies that KGH delegation reverts if the validator status is exited.

456-462: LGTM! Jail condition test is correctly implemented.

The test_delegateKgh_validatorInJail_reverts function correctly verifies that KGH delegation reverts if the validator is in jail.

465-482: LGTM! Batch KGH delegation test is correctly implemented.

The test_delegateKghBatch_succeeds function correctly verifies the batch delegation process and state changes for KGH tokens.

485-500: LGTM! Batch boosted reward claim test is correctly implemented.

The test_delegateKghBatch_claimBoostedReward_succeeds function correctly verifies the claim of boosted rewards after batch KGH delegation.

502-506: LGTM! Validator status condition test is correctly implemented.

The test_delegateKghBatch_validatorStatusNone_reverts function correctly verifies that batch KGH delegation reverts if the validator status is none.

508-515: LGTM! Exited status condition test is correctly implemented.

The test_delegateKghBatch_validatorStatusExited_reverts function correctly verifies that batch KGH delegation reverts if the validator status is exited.

517-525: LGTM! Jail condition test is correctly implemented.

The test_delegateKghBatch_validatorInJail_reverts function correctly verifies that batch KGH delegation reverts if the validator is in jail.

527-533: LGTM! Zero token IDs test is correctly implemented.

The test_delegateKghBatch_zeroTokenIds_reverts function correctly verifies that batch KGH delegation reverts if the token IDs array is empty.

535-549: LGTM! Undelegation test is correctly implemented.

The test_undelegate_succeeds function correctly verifies the undelegation process and state changes.

551-573: LGTM! Multiple delegators undelegation test is correctly implemented.

The test_undelegate_severalDelegators_succeeds function correctly verifies the undelegation process and state changes for multiple delegators.

575-586: LGTM! Validator tree removal test is correctly implemented.

The test_undelegate_removedFromValidatorTree_succeeds function correctly verifies the removal of a validator from the tree upon undelegation.
op-e2e/setup.go (8)
112-115: LGTM! Consistency checks for TrustedValidator addresses.

The added equality checks ensure that the TrustedValidator addresses are consistent with the deployment configuration.

253-258: LGTM! New fields EnableChallenge and ValidatorVersion added.

The new fields enhance the configuration capabilities of the system, allowing for more flexible validator setups.

311-313: LGTM! New fields Challenger and Guardian added.

The new fields enhance the system's capability to manage different validator roles effectively.

407-416: LGTM! Proper stopping logic for Challenger and Guardian.

The added logic ensures that the Challenger and Guardian roles are stopped correctly, handling errors appropriately.

Line range hint 817-911: LGTM! Enhanced validator node setup and challenge system initialization.

The added logic for setting up validator nodes and initializing the challenge system is well-integrated and enhances the system's capabilities.

Line range hint 963-1062: LGTM! New method StartChallengeSystem for managing validator roles.

The method provides a structured approach to managing the Challenger and Guardian roles, ensuring proper configuration and initialization.

1118-1120: LGTM! New method ValidatorHelper added.

The method provides utility functions for validator operations, enhancing the system's functionality.

69-69: Verify the necessity of the valhelper import.

Ensure that the valhelper package is used in the code. If not, consider removing it to avoid unnecessary imports.

Verification successful

The valhelper import is necessary and used in the codebase.

The valhelper package is actively used in the op-e2e/setup.go file and other related files for handling validator versions and registration processes. No changes are needed regarding this import.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify the usage of the `valhelper` import in the code.
rg --type go 'valhelper' -A 5
Length of output: 21080
packages/contracts/contracts/L1/Colosseum.sol (11)

124-124: Verify the implications of changing segmentsLengths to public.

Changing the visibility of segmentsLengths from internal to public allows external access. Ensure that this does not expose sensitive data or lead to unintended side effects.

139-139: Verify the implications of changing deletedOutputs to public.

Changing the visibility of deletedOutputs from internal to public allows external access. Ensure that this does not expose sensitive data or lead to unintended side effects.

412-426: Verify the logic in the initialize function.

Ensure that the logic for checking the length of _segmentsLengths and validating the sum is correct and efficient. Verify that the conditions for reverting are appropriate.

Line range hint 442-490: Verify the logic and error handling in createChallenge.

Ensure that the new logic for handling genesis output, validator status, and creation period is correct. Verify that the error handling is comprehensive and appropriate.

Line range hint 519-547: Verify the logic in the bisect function.

Ensure that the logic for verifying the expected sender and updating segments is correct. Verify that the checks for output finalization are appropriate.

Line range hint 576-650: Verify the logic in the proveFault function.

Ensure that the logic for ZK proof verification and state updates is correct. Verify that the checks for challenge status and public input validation are appropriate.

685-687: Verify the logic in the cancelChallenge function.

Ensure that the logic for checking output deletion and challenge status is correct. Verify that the conditions for canceling a challenge are appropriate.

742-760: Verify the logic in the forceDeleteOutput function.

Ensure that the logic for checking output deletion and security council verification is correct. Verify that the conditions for forcefully deleting an output are appropriate.

1095-1099: Verify the retrieval logic in getSegments.

Ensure that the logic for retrieving challenge segments from the challenges mapping is correct.

1114-1114: Verify the retrieval logic in getStatus.

Ensure that the logic for retrieving the challenge status from the challenges mapping is correct.

1125-1127: Verify the logic in isInCreationPeriod.

Ensure that the logic for checking if the current timestamp is within the creation period is correct.
packages/contracts/.storage-layout (4)
38-41: Addition of _vaults mapping in AssetManager contract.

The addition of _vaults expands asset management capabilities within the contract.

Ensure that this addition does not introduce any storage collision issues, especially if the contract is upgradeable.

Run the following script to verify storage collision:

Verification successful

Verification of _vaults mapping in AssetManager contract

The _vaults mapping is correctly implemented as mapping(address => Vault) internal _vaults; in the AssetManager contract. No immediate storage collision issues are evident. However, ensure that the Vault structure is defined as expected and verify manually if further confirmation is needed.

Location: packages/contracts/contracts/L1/AssetManager.sol
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify storage collision for `AssetManager` contract.

# Test: Check for storage collision. Expect: No collision.
rg --type solidity 'contract AssetManager' -A 20 | rg 'mapping$address => struct IAssetManager.Vault$'
Length of output: 139

Script:
#!/bin/bash
# Description: Verify the addition of `_vaults` mapping in the `AssetManager` contract and check for storage collisions.

# Search for the `AssetManager` contract and the `_vaults` mapping.
rg 'contract AssetManager' -A 100
rg 'mapping$address => struct IAssetManager.Vault$'
Length of output: 17062
16-16: Addition of deletedOutputs mapping in Colosseum contract.

The addition of deletedOutputs enhances state management related to checkpoint outputs.

Ensure that this addition does not introduce any storage collision issues, especially if the contract is upgradeable.

Run the following script to verify storage collision:

102-109: Addition of nextFinalizeOutputIndex in L2OutputOracle contract.

The addition of nextFinalizeOutputIndex improves the management of output finalization processes.

Ensure that this addition does not introduce any storage collision issues, especially if the contract is upgradeable.

Run the following script to verify storage collision:

Verification successful

No storage collision detected for nextFinalizeOutputIndex in L2OutputOracle contract.

The addition of nextFinalizeOutputIndex is correctly implemented, and no storage collision issues are present. The storage layout maintains a logical order, ensuring safe contract upgrades.

Confirmed storage variables: startingBlockNumber, startingTimestamp, l2Outputs, nextFinalizeOutputIndex.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify storage collision for `L2OutputOracle` contract.

# Test: Check for storage collision. Expect: No collision.
rg --type solidity 'contract L2OutputOracle' -A 20 | rg 'uint256'
Length of output: 98

Script:
#!/bin/bash
# Description: Verify storage collision for `L2OutputOracle` contract using ast-grep.

# Search for the `L2OutputOracle` contract and list its storage variables.
ast-grep --lang solidity --pattern $'contract L2OutputOracle { $$$ }'
Length of output: 187

Script:
#!/bin/bash
# Description: Verify storage collision for `L2OutputOracle` contract using rg.

# Search for the `L2OutputOracle` contract in Solidity files and list its storage variables.
rg 'contract L2OutputOracle' -A 50 --type-add 'solidity:*.sol' --type solidity
Length of output: 29231
46-52: Enhancements in ValidatorManager contract storage layout.

The additions of _nextPriorityValidator, _validatorTree, _validatorInfo, _jail, and _pendingChallengeReward enhance validator management.

Ensure that these additions do not introduce any storage collision issues, especially if the contract is upgradeable.

Run the following script to verify storage collision:
packages/contracts/contracts/test/ValidatorManager.t.sol (56)

14-56: LGTM: MockL2OutputOracle implementation.

The mock class is correctly extending L2OutputOracle and provides additional methods for testing.

58-72: LGTM: MockValidatorManager implementation.

The mock class is correctly extending ValidatorManager and provides additional methods for testing.

74-186: Ensure comprehensive test coverage for constructor logic.

The constructor tests verify the initialization of contract parameters. Ensure all edge cases are covered.

188-192: Verify constructor parameter validation logic.

The test checks for invalid constructor parameters. Ensure this logic is consistent with the contract's requirements.

194-218: Verify validator registration logic.

The test covers successful registration and activation of validators. Ensure all conditions for registration are tested.

220-236: Verify registration with insufficient assets.

The test checks registration with assets below the activation threshold. Ensure this logic aligns with contract requirements.

238-242: Verify registration restrictions for contract callers.

The test ensures that contract callers cannot register as validators. Confirm this restriction is necessary.

244-248: Verify registration restrictions for different origins.

The test ensures that different origin addresses cannot register. Confirm this restriction is necessary.

250-259: Verify re-registration logic for initiated validators.

The test checks that already initiated validators cannot re-register. Ensure this logic aligns with contract requirements.

261-268: Verify registration with small asset amounts.

The test checks registration with assets below the minimum required amount. Ensure this logic aligns with contract requirements.

270-277: Verify registration with excessive commission rates.

The test checks registration with commission rates exceeding the maximum allowed. Ensure this logic aligns with contract requirements.

279-286: Verify registration with zero address for withdrawal.

The test checks registration with a zero address for withdrawal. Ensure this logic aligns with contract requirements.

288-294: Verify activation logic for non-validators.

The test checks that non-validators cannot be activated. Ensure this logic aligns with contract requirements.

296-302: Verify activation logic for registered validators.

The test checks that registered validators cannot be activated without meeting conditions. Ensure this logic aligns with contract requirements.

304-314: Verify activation logic for exited validators.

The test checks that exited validators cannot be activated. Ensure this logic aligns with contract requirements.

316-335: Verify activation logic for jailed validators.

The test checks that jailed validators cannot be activated. Ensure this logic aligns with contract requirements.

337-343: Verify re-activation logic for already activated validators.

The test checks that already activated validators cannot be re-activated. Ensure this logic aligns with contract requirements.

345-361: Verify successful activation logic.

The test covers successful activation of validators. Ensure all conditions for activation are tested.

363-413: Verify reward distribution logic after L2 output submission.

The test checks reward distribution after L2 output submission. Ensure all reward calculations are correct.

415-434: Verify reward distribution to Security Council.

The test checks reward distribution to the Security Council. Ensure this logic aligns with contract requirements.

436-483: Verify priority validator update logic.

The test checks updating the priority validator after L2 output submission. Ensure this logic aligns with contract requirements.

485-518: Verify jailing logic after L2 output submission.

The test checks jailing logic after L2 output submission. Ensure this logic aligns with contract requirements.

520-544: Verify no submission count reset logic.

The test checks resetting the no submission count after L2 output submission. Ensure this logic aligns with contract requirements.

546-550: Verify sender restrictions for afterSubmitL2Output.

The test checks that only allowed senders can call afterSubmitL2Output. Ensure this restriction is necessary.

552-568: Verify commission change initiation logic.

The test checks initiating commission changes. Ensure all conditions for initiation are tested.

570-580: Verify commission change initiation for exited validators.

The test checks that exited validators cannot initiate commission changes. Ensure this logic aligns with contract requirements.

582-588: Verify commission change initiation for jailed validators.

The test checks that jailed validators cannot initiate commission changes. Ensure this logic aligns with contract requirements.

590-596: Verify commission change initiation with excessive rates.

The test checks initiating commission changes with excessive rates. Ensure this logic aligns with contract requirements.

598-605: Verify commission change initiation with same rates.

The test checks initiating commission changes with the same rates. Ensure this logic aligns with contract requirements.

607-625: Verify commission change finalization logic.

The test checks finalizing commission changes. Ensure all conditions for finalization are tested.

627-639: Verify commission change finalization for exited validators.

The test checks that exited validators cannot finalize commission changes. Ensure this logic aligns with contract requirements.

642-647: Verify commission change finalization for jailed validators.

The test checks that jailed validators cannot finalize commission changes. Ensure this logic aligns with contract requirements.

650-656: Verify commission change finalization delay logic.

The test checks that commission changes cannot be finalized before the delay. Ensure this logic aligns with contract requirements.

658-664: Verify commission change finalization without initiation.

The test checks that commission changes cannot be finalized without initiation. Ensure this logic aligns with contract requirements.

666-681: Verify unjailing logic.

The test checks unjailing logic. Ensure all conditions for unjailing are tested.

683-687: Verify unjailing logic for non-jailed validators.

The test checks that non-jailed validators cannot be unjailed. Ensure this logic aligns with contract requirements.

689-695: Verify unjailing logic for incorrect sender.

The test checks that only the jailed validator can unjail themselves. Ensure this logic aligns with contract requirements.

697-703: Verify unjailing logic before period elapses.

The test checks that unjailing cannot occur before the jail period elapses. Ensure this logic aligns with contract requirements.

705-713: Verify bonding logic for validators.

The test checks bonding logic for validators. Ensure all conditions for bonding are tested.

715-721: Verify bonding restrictions for non-Colosseum callers.

The test checks that only the Colosseum can bond validators. Ensure this restriction is necessary.

723-735: Verify unbonding logic for validators.

The test checks unbonding logic for validators. Ensure all conditions for unbonding are tested.

737-748: Verify unbonding restrictions for non-Colosseum callers.

The test checks that only the Colosseum can unbond validators. Ensure this restriction is necessary.

750-820: Verify slashing logic.

The test checks slashing logic for validators. Ensure all conditions for slashing are tested.

822-826: Verify slashing restrictions for non-Colosseum callers.

The test checks that only the Colosseum can slash validators. Ensure this restriction is necessary.

828-877: Verify slash reversion logic.

The test checks slash reversion logic for validators. Ensure all conditions for reversion are tested.

879-883: Verify slash reversion restrictions for non-Colosseum callers.

The test checks that only the Colosseum can revert slashes. Ensure this restriction is necessary.

885-891: Verify submission eligibility check for priority round.

The test checks submission eligibility for the priority round. Ensure all conditions for eligibility are tested.

893-902: Verify submission eligibility check for public round.

The test checks submission eligibility for the public round. Ensure all conditions for eligibility are tested.

904-907: Verify submission eligibility restrictions for non-L2OO senders.

The test checks that only the L2OO can check submission eligibility. Ensure this restriction is necessary.

909-916: Verify submission eligibility for non-selected validators.

The test checks that non-selected validators cannot check submission eligibility. Ensure this logic aligns with contract requirements.

918-924: Verify submission eligibility for unsatisfied conditions.

The test checks that validators not satisfying conditions cannot check submission eligibility. Ensure this logic aligns with contract requirements.

926-934: Verify submission eligibility in public round for unsatisfied conditions.

The test checks that validators not satisfying conditions cannot check submission eligibility in the public round. Ensure this logic aligns with contract requirements.

936-944: Verify submission eligibility for jailed validators.

The test checks that jailed validators cannot check submission eligibility. Ensure this logic aligns with contract requirements.

946-956: Verify submission eligibility in public round for jailed validators.

The test checks that jailed validators cannot check submission eligibility in the public round. Ensure this logic aligns with contract requirements.

958-965: Verify status retrieval for registered validators.

The test checks status retrieval for registered validators. Ensure all conditions for status retrieval are tested.

967-988: Verify total weight calculation for activated validators.

The test checks total weight calculation for activated validators. Ensure all conditions for weight calculation are tested.
packages/contracts/contracts/test/KromaPortal.t.sol (2)
99-99: Cosmetic change approved for readability.

The addition of spaces around the exponentiation operator in vm.deal(alice, 2 ** 64); improves readability without affecting functionality.

335-335: Verify the implementation of warpToSubmitTime().

The use of warpToSubmitTime() abstracts time manipulation logic, which can enhance readability and maintainability. Ensure that this function encapsulates the correct logic for time warping.

Run the following script to verify the implementation of warpToSubmitTime():

Verification successful

The implementation of warpToSubmitTime() correctly encapsulates time manipulation logic.

The function warpToSubmitTime() is implemented in CommonTest.t.sol and uses vm.warp(oracle.nextOutputMinL2Timestamp()), which effectively centralizes the logic for setting the time to the appropriate oracle-provided timestamp. This abstraction enhances both readability and maintainability.

Location: packages/contracts/contracts/test/CommonTest.t.sol
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the implementation of `warpToSubmitTime()`.

# Test: Search for the function implementation. Expect: Correct encapsulation of time warping logic.
ast-grep --lang solidity --pattern 'function warpToSubmitTime() { $$$ }'
Length of output: 191

Script:
#!/bin/bash
# Description: Use ripgrep to find the implementation of `warpToSubmitTime()` in Solidity files.

# Test: Search for the function definition in Solidity files.
rg 'function warpToSubmitTime' --glob '*.sol' -A 5
Length of output: 532
packages/contracts/.gas-snapshot (4)

1-1: Test case test_bondValidatorKro_callerNotValMgr_reverts looks good!

The test case is correctly named and its purpose is clear. It checks the behavior when a non-validator manager tries to bond a validator.

467-467: Test case test_activateValidator_alreadyActivated_reverts looks good!

The test case is well-named and aligns with its intended purpose of checking the behavior when trying to activate an already activated validator.

521-521: Test case test_addPendingBond_insufficientBalances_reverts looks good!

The test case is appropriately named and its purpose is clear, checking the behavior when adding a pending bond with insufficient balances.

68-68: Test case test_bisect_cancelChallenge_succeeds looks good!

The test case is well-named and aligns with its intended purpose of checking the successful cancellation of a challenge.
kroma-bindings/bindings/colosseum_more.go (2)
12-12: Verify new structures and mappings in ColosseumStorageLayoutJSON.

The JSON updates include new mappings and types, such as Challenge and CheckpointOutput. Ensure that these structures are correctly defined and consistent with the rest of the codebase.

Run the following script to verify the definitions of new structures:

Verification successful

Structures Verified in ColosseumStorageLayoutJSON.

The Challenge and CheckpointOutput structures are correctly defined in the codebase, ensuring consistency with the JSON updates in ColosseumStorageLayoutJSON.

Challenge struct is defined in packages/contracts/contracts/libraries/Types.sol.

CheckpointOutput struct is defined in packages/contracts/contracts/libraries/Types.sol.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the definitions of `Challenge` and `CheckpointOutput` structures.

# Test: Search for the structure definitions. Expect: Definitions of `Challenge` and `CheckpointOutput`.
ast-grep --lang solidity --pattern $'struct Challenge {
  $$$
}'
ast-grep --lang solidity --pattern $'struct CheckpointOutput {
  $$$
}'
Length of output: 372

Script:
#!/bin/bash
# Description: Verify the definitions of `Challenge` and `CheckpointOutput` structures using ripgrep.

# Search for the definition of the `Challenge` struct in Solidity files.
rg 'struct Challenge' --type solidity -A 10

# Search for the definition of the `CheckpointOutput` struct in Solidity files.
rg 'struct CheckpointOutput' --type solidity -A 10
Length of output: 164

Script:
#!/bin/bash
# Description: Verify the definitions of `Challenge` and `CheckpointOutput` structures using ripgrep.

# Search for the definition of the `Challenge` struct in Solidity files.
rg 'struct Challenge' --glob '*.sol' -A 10

# Search for the definition of the `CheckpointOutput` struct in Solidity files.
rg 'struct CheckpointOutput' --glob '*.sol' -A 10
Length of output: 1723
16-16: Verify the impact of changes in ColosseumDeployedBin.

The binary representation has undergone extensive modifications. Ensure that these changes are intentional and verify their impact on the contract's functionality.

Run the following script to verify the contract's functionality:
kroma-chain-ops/genesis/config.go (5)

168-169: Verify the deployment logic for governance tokens.

The introduction of GovernanceTokenNotUseCreate2 and related fields indicates a change in the deployment strategy. Ensure that the deployment logic is updated accordingly to handle these new configurations.

276-301: Verify the validator management logic.

The addition of new fields related to validator management provides more granular control over validator operations. Ensure that the validator management logic is updated to utilize these new configurations effectively.

302-309: Verify the asset management logic.

The addition of new fields related to asset management, such as AssetManagerKgh and AssetManagerVault, expands the configuration options. Ensure that the asset management logic is updated to handle these new configurations effectively.

Line range hint 445-587: Verify the validation logic in Check method.

The Check method has been expanded to include validations for the new fields. Ensure that the validation logic correctly enforces the required conditions for these fields.

Line range hint 847-864: Verify the deployment configuration logic in L1Deployments.

The addition of new fields related to governance tokens, asset managers, and validator managers expands the deployment configuration. Ensure that the deployment configuration logic is updated to handle these new components effectively.

packages/contracts/contracts/test/CommonTest.t.sol (8)

7-9: Imports look good.

The new imports for IERC20, ERC721, and their interfaces are appropriate for the mock contracts.

181-187: MockKro contract implementation is correct.

The MockKro contract provides a simple mint function for testing ERC20 interactions.

189-195: MockKgh contract implementation is correct.

The MockKgh contract provides a simple mint function for testing ERC721 interactions.

261-265: Event modification is appropriate.

The addition of the newSubmitter parameter to the OutputReplaced event enhances its utility.

268-269: Function simplification is beneficial.

The warpToSubmitTime function's simplification enhances clarity and reduces potential errors.

409-431: Internal functions improve modularity.

The _registerValidator, _submitL2OutputV1, and _submitL2OutputV2 functions enhance the modularity and testability of the code.

434-469: Contract changes emphasize upgradeability.

The ValidatorSystemUpgrade_Initializer contract's new setup logic reflects a focus on upgradeability and modularity.

Line range hint 201-245: State variable additions and constructor parameters look good.

The new state variables and constructor parameters for AssetManager and ValidatorManager are appropriate for managing assets and validators.

However, verify the initialization logic to ensure all parameters are correctly set.

Run the following script to verify the initialization logic:
packages/contracts/contracts/test/Colosseum.t.sol (9)
59-63: Validate public input hashing logic.

The hashPublicInput function computes a hash based on the public input proof. Ensure that the hashing logic is correct and aligns with the expected behavior.

Run the following script to verify the hashing logic:

312-327: Ensure constructor test covers all scenarios.

The test_constructor_succeeds function tests the constructor of Colosseum. Ensure that the test covers all scenarios and edge cases.

Run the following script to verify the constructor test:

Line range hint 222-238: Check bisecting logic for correctness.

The _bisect function bisects a challenge and retrieves the new challenge state. Ensure that the bisecting logic is correct and handles all scenarios.

Run the following script to verify the bisecting logic:

Verification successful

Bisecting Logic Verified

The _bisect function in Colosseum.t.sol retrieves a challenge, calculates segment details, and calls the bisect method. The logic aligns with the expected operations and appears correct. If further verification is needed, consider additional tests for edge cases.

Location: packages/contracts/contracts/test/Colosseum.t.sol
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the bisecting logic in `_bisect`.

# Test: Search for the `_bisect` function. Expect: Correct implementation.
rg --type solidity -A 10 $'function _bisect'
Length of output: 78

Script:
#!/bin/bash
# Locate all Solidity files and search for the `_bisect` function within them.
fd -e sol | xargs rg -A 10 'function _bisect'
Length of output: 2391
Line range hint 68-97: Verify the setup logic for MockColosseum.

The setUp function initializes the MockColosseum and upgrades the proxy. Ensure that the setup logic correctly initializes all components and dependencies.

Run the following script to verify the setup logic:

Verification successful

Setup Logic for MockColosseum Verified

The setUp function in Colosseum.t.sol correctly initializes the MockColosseum and upgrades the proxy as expected. No issues were found with the setup logic.

The MockColosseum is instantiated with the appropriate parameters.

The proxy upgrade is performed correctly.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the setup logic for `MockColosseum`.

# Test: Search for the `setUp` function. Expect: Correct initialization of components.
rg --type solidity -A 10 $'function setUp()'
Length of output: 78

Script:
#!/bin/bash
# Description: Verify the setup logic for `MockColosseum`.

# Find all Solidity files and search for the `setUp` function.
fd -e sol | xargs rg -A 10 'function setUp()'
Length of output: 64637
329-333: Validate initialization test logic.

The test_initialize_succeeds function tests the initialization logic of Colosseum. Ensure that the test logic is correct and covers all scenarios.

Run the following script to verify the initialization test logic:

210-215: Ensure challenge creation logic is robust.

The _createChallenge function creates a challenge and retrieves it using getChallenge. Ensure that the challenge creation logic is robust and handles edge cases.

Run the following script to verify the challenge creation logic:

Verification successful

Challenge creation logic appears robust.

The _createChallenge function is well-tested across various scenarios, including success cases and edge conditions, indicating robust logic.

The function is thoroughly tested in packages/contracts/contracts/test/Colosseum.t.sol.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the challenge creation logic in `_createChallenge`.

# Test: Search for the `_createChallenge` function. Expect: Robust logic.
rg --type solidity -A 10 $'function _createChallenge'
Length of output: 87

Script:
#!/bin/bash
# Description: Search for the `_createChallenge` function in Solidity files.

# Test: Search for the `_createChallenge` function. Expect: Robust logic.
rg 'function _createChallenge' -A 10 --glob '*.sol'
Length of output: 1122

Script:
#!/bin/bash
# Description: Search for additional logic related to `_createChallenge` and its test cases.

# Search for references to `_createChallenge` in Solidity files to find additional logic or calls.
rg '_createChallenge' --glob '*.sol'

# Search for test cases related to `_createChallenge` in test files.
rg '_createChallenge' --glob '*test*.sol'
Length of output: 5376
52-57: Ensure bisecting logic is correctly implemented.

The isAbleToBisect function checks if a challenge can be bisected. Ensure that the logic in _isAbleToBisect is correct and covers all edge cases.

Run the following script to verify the bisecting logic:

Verification successful

Bisecting logic is correctly implemented.

The _isAbleToBisect function checks if the next segment size of a challenge is greater than 1, which is a valid condition for bisecting. The logic is used consistently across the codebase, ensuring proper handling of challenge states.

_isAbleToBisect function: Returns true if the next segment size is greater than 1.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the bisecting logic in `isAbleToBisect`.

# Test: Search for the `_isAbleToBisect` function. Expect: Correct implementation.
rg --type solidity -A 5 $'_isAbleToBisect'
Length of output: 74

Script:
#!/bin/bash
# Correcting the file type to search for the `_isAbleToBisect` function in Solidity files.

# Test: Search for the `_isAbleToBisect` function. Expect: Correct implementation.
rg --type sol -A 5 $'_isAbleToBisect'
Length of output: 64

Script:
#!/bin/bash
# Finding all Solidity files and searching for the `_isAbleToBisect` function.

# Locate all Solidity files and search for the `_isAbleToBisect` function.
fd --extension sol --exec rg -A 5 '_isAbleToBisect' {}
Length of output: 1326
245-255: Validate fault proving logic.

The _proveFault function proves a fault and checks the challenge status. Ensure that the fault proving logic is correct and aligns with the expected behavior.

Run the following script to verify the fault proving logic:

Verification successful

Fault Proving Logic Verified

The _proveFault function in the code correctly implements the logic for proving a fault. It retrieves necessary data, detects a fault, and verifies the challenge status and output, aligning with the expected behavior.

The function interacts with the oracle and mockColosseum to get the previous snapshot and challenge.

It detects a fault and proves it, ensuring the challenge status is updated correctly.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the fault proving logic in `_proveFault`.

# Test: Search for the `_proveFault` function. Expect: Correct implementation.
rg --type solidity -A 10 $'function _proveFault'
Length of output: 82

Script:
#!/bin/bash
# Find all Solidity files and search for the `_proveFault` function
fd --extension sol --exec rg 'function _proveFault' {}
Length of output: 84

Script:
#!/bin/bash
# Find all Solidity files and search for the `_proveFault` function with more context
fd --extension sol --exec rg 'function _proveFault' -A 20 {}
Length of output: 927
16-43: Ensure constructor parameters are correctly initialized.

The constructor of MockColosseum initializes its parent contract with several parameters. Verify that all parameters are correctly passed and initialized.

Run the following script to verify the constructor parameters:

Verification successful

Constructor parameters are correctly initialized.

The constructor of MockColosseum correctly initializes its parameters and passes them to the parent Colosseum contract. No issues were found with the parameter initialization.

L2OutputOracle _l2Oracle

ZKVerifier _zkVerifier

uint256 _submissionInterval

uint256 _creationPeriodSeconds

uint256 _bisectionTimeout

uint256 _provingTimeout

bytes32 _dummyHash

uint256 _maxTxs

uint256[] memory _segmentsLengths
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the constructor parameters of `MockColosseum`.

# Test: Search for the constructor usage. Expect: Correct initialization of parameters.
rg --type solidity -A 10 $'constructor('
Length of output: 116

Script:
#!/bin/bash
# Description: Verify the constructor parameters of `MockColosseum`.

# Test: Search for the constructor usage. Expect: Correct initialization of parameters.
rg --type solidity -A 10 'constructor('
Length of output: 116

Script:
#!/bin/bash
# Description: Verify the constructor parameters of `MockColosseum`.

# Test: Search for the constructor usage. Expect: Correct initialization of parameters.
rg --type solidity -A 10 'contract MockColosseum'
Length of output: 84

Script:
#!/bin/bash
# Description: Verify the constructor parameters of `MockColosseum`.

# Test: Search for the constructor usage. Expect: Correct initialization of parameters.
rg --type sol -A 10 'contract MockColosseum'
Length of output: 74

Script:
#!/bin/bash
# Description: Verify the constructor parameters of `MockColosseum`.

# Test: Search for the constructor usage. Expect: Correct initialization of parameters.
rg 'contract MockColosseum' --glob '*.sol' -A 10
Length of output: 962
kroma-bindings/bindings/l2outputoracle.go (9)

329-344: LGTM! The VALIDATORMANAGER function is correctly implemented.

The function retrieves the address of the validator manager and handles errors appropriately.

701-716: LGTM! The NextFinalizeOutputIndex function is correctly implemented.

The function retrieves the next finalize output index and handles errors appropriately.

763-778: LGTM! The NextOutputMinL2Timestamp function is correctly implemented.

The function retrieves the next output minimum L2 timestamp and handles errors appropriately.

1175-1180: LGTM! The OutputReplaced event modification is correctly implemented.

The addition of the indexed newSubmitter parameter allows for more granular event filtering.

1180-1194: LGTM! The FilterOutputReplaced function is correctly implemented.

The function now includes filtering by newSubmitter, enhancing its functionality.

1201-1215: LGTM! The WatchOutputReplaced function is correctly implemented.

The function now includes watching by newSubmitter, enhancing its functionality.

1247-1249: LGTM! The ParseOutputReplaced function is correctly implemented.

The function reflects changes in the OutputReplaced event structure.

929-934: LGTM! The SetNextFinalizeOutputIndex function is correctly implemented.

The function sets the next finalize output index and handles transactions appropriately.

936-948: LGTM! The session function for SetNextFinalizeOutputIndex is correctly implemented.

The function sets the next finalize output index within a session.

op-e2e/system_test.go (3)

166-244: LGTM!

The TestL2OutputSubmitterV2 function is well-structured and correctly implements the test logic for the L2 output submitter. The use of context and ticker for timeouts is appropriate.

295-405: LGTM!

The TestValidatorSystemUpgradeToV2 function is well-structured and correctly implements the test logic for validator system upgrades and reward distribution. The logic for tracking assets and rewards is correctly implemented.

1864-1994: LGTM!

The TestChallengerTimeoutByGuardian function is well-structured and correctly implements the test logic for handling challenger timeouts by a guardian. The logic for handling timeouts and verifying the guardian's actions is correctly implemented.
kroma-bindings/bindings/validatorpool.go (3)
453-468: New function TERMINATEOUTPUTINDEX added successfully.

The function is correctly implemented to retrieve the terminate output index from the contract.

639-654: New function IsTerminated added successfully.

The function is correctly implemented to check the termination status of a specific output index.

Line range hint 47-56: LGTM! But verify the function usage in the codebase.

The addition of _terminateOutputIndex to the DeployValidatorPool function is correctly implemented.

However, ensure that all function calls to DeployValidatorPool match the new signature.

Run the following script to verify the function usage:

Verification successful

Function Usage Verified: All calls to DeployValidatorPool match the new signature.

The function DeployValidatorPool is correctly used in op-e2e/e2eutils/setup.go, with all required parameters, including the newly added _terminateOutputIndex. No further action is needed.

Verified function call in op-e2e/e2eutils/setup.go.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `DeployValidatorPool` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'DeployValidatorPool'
Length of output: 1272

Script:
#!/bin/bash
# Description: Extract more lines around the function call to `DeployValidatorPool` in `setup.go` to verify the arguments.
rg --type go -A 10 -B 2 $'DeployValidatorPool' op-e2e/e2eutils/setup.go
Length of output: 551
kroma-validator/l2_output_submitter.go (14)

43-47: LGTM: Enhanced struct capability and consistency.

The addition of valMgrContract and assetMgrContract fields expands the struct's functionality. The renaming of fields improves consistency.

62-95: LGTM: Proper initialization of new contracts.

The method correctly initializes the new contract fields and handles errors consistently.

Line range hint 105-161: LGTM: Enhanced robustness in configuration initialization.

The method now fetches bond amounts from different contracts, reflecting a more robust validation mechanism.

224-247: LGTM: Improved decision-making for output submission.

The method now fetches the next output index and determines if submission is permissible, enhancing its functionality.

255-266: LGTM: Alignment with expanded functionality.

The addition of the outputIndex parameter aligns with the expanded functionality of handling multiple outputs.

Line range hint 279-314: LGTM: Enhanced decision-making for wait time calculation.

The method now includes an outputIndex parameter and additional logic for calculating wait time, improving its functionality.

322-377: LGTM: Enhanced robustness in output submission checks.

The method now includes an outputIndex parameter and logic to check validator status, ensuring validators are in the correct state before submission attempts.

379-388: LGTM: Necessary addition for handling multiple outputs.

The method fetches the next output index and integrates well with existing logic.

424-426: LGTM: Necessary check for validator pool state.

The method checks if the validator pool is terminated, providing a necessary state check.

428-436: LGTM: Necessary addition for fetching validator status.

The method fetches the validator status, providing necessary information for determining if a validator can submit outputs.

438-447: LGTM: Necessary check for validator jail status.

The method checks if a validator is in jail, providing a necessary state check.

Line range hint 555-633: LGTM: Alignment with expanded functionality.

The method now includes an outputIndex parameter and logic for handling storage keys, aligning with the expanded functionality.

636-637: LGTM: Necessary access to ABI.

The method returns the l2OOABI field, providing necessary access to the ABI for interacting with contracts.

Line range hint 475-512: LGTM: Enhanced functionality for fetching current round.

The method now includes an outputIndex parameter and logic for fetching the next validator address, enhancing its functionality.

packages/contracts/contracts/L1/ValidatorPool.sol (4)

75-79: LGTM: Enhanced lifecycle management with TERMINATE_OUTPUT_INDEX.

The addition of TERMINATE_OUTPUT_INDEX enhances the contract's lifecycle management by tracking the termination point.

182-184: LGTM: Accurate version increment to 1.1.0.

The version increment reflects the significant changes and enhancements made to the contract.

Line range hint 189-214: LGTM: Necessary inclusion of _terminateOutputIndex parameter.

The constructor now includes a _terminateOutputIndex parameter, necessary for initializing the new TERMINATE_OUTPUT_INDEX variable.

611-620: LGTM: Enhanced robustness with isTerminated function.

The function determines if a given output index exceeds the termination index, providing a mechanism for safe interaction with the ValidatorPool.

kroma-validator/cmd/validator/flags.go

packages/contracts/contracts/test/BalancedWeightTree.t.sol

op-e2e/actions/l2_runtime.go

packages/contracts/contracts/test/Colosseum.t.sol

op-e2e/system_test.go

0xHansLee · 2024-08-23T05:16:17Z

Let's merge this PR without squash.

sm-stack and others added 30 commits April 18, 2024 14:22

feat(contracts): add arctangent library

bcafeda

feat(contracts): add AssetManager contract

7bc46e5

feat(contracts): add interface for KGHManager contract

34a2700

feat(contracts): add test codes for AssetManager

15e0c7f

feat(contracts): add Uint128Math library

cef7a30

feat(contracts): add balanced weight tree library

a726010

fix(contracts): check correct minimum timestamp for next output submi…

1c9e2ad

…ssion

feat(contracts): add ValidatorManager contract and refac `AssetMana…

62c9912

…ger`

feat(contracts): handle validator system upgrade in L2OutputOracle

0cb9f85

feat(chain-ops): apply changes with AssetManager and `ValidatorMana…

208cd2a

…ger` to chain-ops

chore(contracts): update bindings and snapshots

0108d07

feat(contracts): apply PR reviews

c39f2da

feat(contracts): upgrade validator system in Colosseum

a96e0a2

refac(contracts): replace all errors with custom error in Colosseum

dbed8b6

feat(contracts): send the challenge loser to jail when slashing

1a693d3

refac(contracts): move interfaces to separate folder

0d94687

refac(contracts): add IAssetManager

85f26e8

test(contracts): modify tests related to slashing

d30a495

test(contracts): add tests for validator system upgrade in Colosseum

edf7208

chore(contracts): update bindings and snapshots

9d1bfff

feat(contracts): apply PR reviews

16802c3

feat(validator): add logics for new validator system

2511cbc

feat(e2e): add action tests for new validator system

852e5b8

chore: set required flags for devnet

0c05940

fix(validator): add real-time update logic for terminate index of Val…

02d5012

…idatorPool

fix(contracts): fix ValidatorPool to check termination by index

333f513

fix(validator): remove inJail status in the validator client

95665a2

feat(validator): apply PR Reviews

8169f41

feat(validator): add subcommands for the new validator system

85c9784

feat(validator): add flags used in subcommands

845b217

0xHansLee and others added 19 commits July 22, 2024 20:20

chore: remove 2 step claim of validator reward. (#356)

b7dd902

feat(contracts): remove KRO in KGH (#358)

f8bbed7

* feat(contracts): remove KRO in KGH * feat(contracts): apply PR reviews

feat(contracts): add withdraw method for validator (#357)

0b00b05

* feat: add withdraw method for validator * feat: apply PR reviews * feat: review applied * feat: review applied * feat: review applied

test(contracts): setup for AssetManager and ValidatorManager (#359)

3f0e5ec

test(contracts): add AssetManager tests (#361)

c0c77dd

feat: change validator client subcommands (#360)

c77accc

* feat: remove dup checks in the process of register * feat: update subcommands for validator system v2 * feat: update README for validator system v2 * feat: apply PR reviews

test: fix validator manager contract tests (#364)

1c302b5

* test: fix validator manager contract tests * feat: review applied * feat: review applied * feat: review applied * feat: move revertSlash test

feat(contracts): modify deploy scripts (#365)

042efa8

* chore(contracts): update bindings and snapshot * feat(contracts): modify deploy scripts * refac(contracts): reduce local vars in `Colosseum` to reduce code size * chore(chain-ops): set `ValidatorPool` termination index to 10 in devnet

test(validator): modify e2e tests related to V2 (#366)

5583cf9

* feat(validator): assert output submission condition before submit tx * test(validator): modify e2e tests related to V2

feat(contracts): auto activate validator when deposit (#367)

ab37de4

fix(validator): modify validator to be able to turn on without V2 (#368)

aca9645

fix(validator): disable validator to be turned on without V2 (#369)

f885f5f

* fix(validator): disable validator to be turned on without V2 * feat(validator): improve bond log * refac(contracts): remove input argument of `tryUnjail`

Merge branch 'refs/heads/dev' into feat/implement-validator-system-v2

75ae796

seolaoh self-assigned this Aug 23, 2024

seolaoh requested review from a team as code owners August 23, 2024 04:51

0xHansLee approved these changes Aug 23, 2024

View reviewed changes

coderabbitai bot reviewed Aug 23, 2024

View reviewed changes

sm-stack approved these changes Aug 23, 2024

View reviewed changes

Pangssu approved these changes Aug 23, 2024

View reviewed changes

seolaoh merged commit 87fc8ab into dev Aug 23, 2024
2 checks passed

seolaoh deleted the feat/implement-validator-system-v2 branch August 23, 2024 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement KRO-based validator system #374

feat: implement KRO-based validator system #374

seolaoh commented Aug 23, 2024 •

edited

Loading

coderabbitai bot commented Aug 23, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

0xHansLee left a comment

coderabbitai bot left a comment

0xHansLee commented Aug 23, 2024

feat: implement KRO-based validator system #374

feat: implement KRO-based validator system #374

Conversation

seolaoh commented Aug 23, 2024 • edited Loading

Description

coderabbitai bot commented Aug 23, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

0xHansLee left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

0xHansLee commented Aug 23, 2024

seolaoh commented Aug 23, 2024 •

edited

Loading

coderabbitai bot commented Aug 23, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)