fix(e2e): fix challenge e2e tests not working properly #273
Conversation
WalkthroughThe changes span across multiple components of a blockchain system, focusing on enhancing the functionality and reliability of the network. Key updates include the improvement of logic for ownership transfer and storage manipulation in smart contracts, refining validation and challenge processes in the layer 2 environment, and extending support for a new module in Geth configurations. Additionally, the updates aim to improve code clarity and consistency through method renaming and error handling adjustments. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (7)
- kroma-chain-ops/genesis/layer_one.go (3 hunks)
- kroma-validator/guardian.go (7 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/setup.go (1 hunks)
- op-e2e/system_test.go (5 hunks)
Additional comments: 10
kroma-chain-ops/genesis/layer_one.go (3)
- 124-124: The added logging messages enhance transparency and are helpful for debugging purposes.
- 121-130: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [127-145]
Ownership transfer logic for
SystemConfigappears correctly implemented. Ensure theProxyAdminaddress is correctly set and assess potential security implications.
- 147-206: Modifications to storage keys for
UpgradeGovernorandSecurityCouncilTokenare carefully implemented. It's crucial to review these changes in the context of the contract behavior to ensure consistency and avoid unintended consequences.op-e2e/actions/l2_runtime.go (1)
- 62-62: The addition of
defaultRuntimesimplifies the setup forl2_challenger_test. Ensure this function is used exclusively in testing contexts and not in production code paths.kroma-validator/guardian.go (1)
- 291-291: Renaming and restructuring methods within the
Guardianstruct for clarity and consistency is a positive change. However, ensure that all references to these methods are updated accordingly across the codebase to prevent any broken functionality. Additionally, consider adding unit tests or enhancing existing ones to cover the changes in method behavior and ensure that the Guardian's functionality remains intact.Verification successful
The verification process has confirmed that the renaming and restructuring of methods within the
Guardianstruct, specificallyisOutputFinalized,CheckConfirmCondition,ValidateL2Output, andConfirmTransaction, have been correctly reflected across the codebase. The changes are consistent with the review comment, indicating that all references to these methods have been updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to renamed methods are updated. rg --type go "isOutputFinalized|CheckConfirmCondition|ValidateL2Output|ConfirmTransaction"Length of output: 21752
op-e2e/system_test.go (5)
- 46-47: The import statements for
kroma-network/kroma/kroma-bindings/bindingsandkroma-network/kroma/kroma-bindings/predeploysare added. Ensure that these packages are correctly referenced and accessible within the project's dependency management system.- 1472-1473: The retrieval of the
SecurityCouncilABI usingbindings.SecurityCouncilMetaData.GetAbi()is a critical step for further operations. Ensure that the ABI is correctly loaded and matches the deployed contract's interface to avoid runtime errors.- 1501-1523: The function
findTransactionIdis used to find a specific transaction ID based on theValidationRequestedevent. It's important to ensure that this logic correctly filters and retrieves the intended transaction ID, considering potential edge cases where multiple events might be found.- 1498-1546: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1521-1557]
The logic within the loop that waits for the challenge status to become
READY_TO_PROVEand then checks for the challenge's success is crucial. Ensure that the timeout and retry logic is appropriately set to handle scenarios where the expected status might take longer than anticipated or might not be reached due to unforeseen issues.
- 1498-1546: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1501-1557]
The entire challenge test logic, including waiting for the challenge status to change and verifying the outcome, is complex and involves multiple external calls and state checks. It's essential to ensure that error handling is robust and that the test accurately reflects the intended behavior of the system under test.
Consider adding more detailed error messages and checks for intermediate states to improve the test's diagnostic capability in case of failures.
seolaoh
force-pushed
the
fix/fix-challenge-e2e
branch
from
adf74ec to
fa0f72d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- kroma-chain-ops/genesis/layer_one.go (3 hunks)
- kroma-validator/guardian.go (7 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/setup.go (3 hunks)
- op-e2e/system_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (7)
- kroma-chain-ops/genesis/layer_one.go
- kroma-validator/guardian.go
- op-e2e/actions/l2_challenger_test.go
- op-e2e/actions/l2_guardian.go
- op-e2e/actions/l2_runtime.go
- op-e2e/setup.go
- op-e2e/system_test.go
seolaoh
force-pushed
the
fix/fix-challenge-e2e
branch
from
fa0f72d to
8382b98
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- kroma-chain-ops/genesis/layer_one.go (3 hunks)
- kroma-validator/guardian.go (8 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/setup.go (3 hunks)
- op-e2e/system_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (7)
- kroma-chain-ops/genesis/layer_one.go
- kroma-validator/guardian.go
- op-e2e/actions/l2_challenger_test.go
- op-e2e/actions/l2_guardian.go
- op-e2e/actions/l2_runtime.go
- op-e2e/setup.go
- op-e2e/system_test.go
seolaoh
force-pushed
the
fix/fix-challenge-e2e
branch
2 times, most recently
from
e5235e4 to
8907e09
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- kroma-chain-ops/genesis/layer_one.go (3 hunks)
- kroma-validator/guardian.go (8 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/e2eutils/geth/geth.go (1 hunks)
- op-e2e/setup.go (1 hunks)
- op-e2e/system_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (6)
- kroma-validator/guardian.go
- op-e2e/actions/l2_challenger_test.go
- op-e2e/actions/l2_guardian.go
- op-e2e/actions/l2_runtime.go
- op-e2e/setup.go
- op-e2e/system_test.go
Additional comments: 5
op-e2e/e2eutils/geth/geth.go (1)
- 78-79: Adding
"kroma"to bothWSModulesandHTTPModulesarrays extends the capabilities of the WebSocket and HTTP interfaces, enabling additional interactions with the Kroma module. This change is crucial for achieving the PR objective of allowing challengers to retrieve trace information from L2 nodes. Ensure that the Kroma module is fully implemented and tested to handle the expected interactions without introducing security or performance issues.kroma-chain-ops/genesis/layer_one.go (4)
- 124-124: The logging statement after updating the storage for
KromaPortalprovides transparency about the changes made during the post-processing of the L1 developer genesis state. This is a good practice for debugging and auditing purposes. However, ensure that logging at this level of detail is appropriate for the production environment, as excessive logging can lead to performance issues and log bloat.- 145-145: The logging statement after transferring ownership of
SystemConfigtoProxyAdminOwneris useful for tracking changes. Similar to the previous comment, consider the appropriateness of this level of logging in production environments.- 173-173: Logging the before and after values of the storage update for
UpgradeGovernoris beneficial for understanding the impact of the post-processing steps. As with previous logging comments, evaluate the necessity and frequency of such logs in a production setting.- 205-205: The detailed logging for the
SecurityCouncilTokenupdates provides insight into the changes made to the contract's storage. This consistency in logging practices across different contract updates is commendable. Again, consider the impact of detailed logging on the system's performance and storage.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- kroma-chain-ops/genesis/layer_one.go (3 hunks)
- kroma-validator/guardian.go (8 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/e2eutils/geth/geth.go (1 hunks)
- op-e2e/system_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (6)
- kroma-validator/guardian.go
- op-e2e/actions/l2_challenger_test.go
- op-e2e/actions/l2_guardian.go
- op-e2e/actions/l2_runtime.go
- op-e2e/e2eutils/geth/geth.go
- op-e2e/system_test.go
Additional comments: 6
kroma-chain-ops/genesis/layer_one.go (6)
- 14-14: The addition of the
cryptopackage suggests new cryptographic operations are being introduced. Ensure that these operations are secure and necessary for the intended functionality.- 17-17: The introduction of
kroma-network/kroma/kroma-bindings/bindingsindicates new bindings are being utilized. Verify that these bindings are up-to-date and correctly implemented to avoid potential integration issues.- 124-124: Logging the post-process update for
KromaPortalis good for transparency and debugging. However, ensure that logging does not expose sensitive information that could be exploited.- 121-130: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [127-145]
Transferring ownership of
SystemConfigtoProxyAdminOwnerfor testing is a significant change. Confirm that this ownership transfer is reversible or appropriate for the testing environment to avoid unintended access control issues.
- 147-173: The manipulation of
_quorumNumeratorHistoryinUpgradeGovernorfor testing purposes is noted. It's crucial to ensure that such changes do not affect the integrity of the governance process in a production environment.- 175-205: Changing the keys of
_totalCheckpointsinSecurityCouncilTokenfor testing is a critical operation. Verify that these changes are isolated to the testing environment and do not impact the token's security or functionality in production.
kroma-chain-ops/genesis/layer_one.go
Outdated
| layout, err = bindings.GetStorageLayout("UpgradeGovernor") | ||
| if err != nil { | ||
| return errors.New("failed to get storage layout for UpgradeGovernor") | ||
| } | ||
|
|
||
| entry, err = layout.GetStorageLayoutEntry("_quorumNumeratorHistory") | ||
| if err != nil { | ||
| return errors.New("failed to get storage layout entry for UpgradeGovernor._quorumNumeratorHistory") | ||
| } |
There was a problem hiding this comment.
I suggest making this code block a function.
func getStorageLayerEntity(contractName, fieldName string) (solc.StorageLayoutEntry, error) {
layout, err := bindings.GetStorageLayout(contractName)
if err != nil {
return solc.StorageLayoutEntry{}, fmt.Errorf("failed to get storage layout for %s", contractName)
}
entry, err := layout.GetStorageLayoutEntry(fieldName)
if err != nil {
return solc.StorageLayoutEntry{}, fmt.Errorf("failed to get storage layout for %s.%s", contractName, fieldName)
}
return entry, nil
}
kroma-chain-ops/genesis/layer_one.go
Outdated
| stateDB.SetState(deployments.UpgradeGovernorProxy, slot, val) | ||
| afterVal := stateDB.GetState(deployments.UpgradeGovernorProxy, slot) |
There was a problem hiding this comment.
What does this code mean?
stateDB.SetState alone seems like it should be enough.
There was a problem hiding this comment.
It's because to log the difference between before value and after value below line 173.
There was a problem hiding this comment.
I understand that the values are different, but isn't that the val variable? I don't understand assigning the afterVal variable with getState.
There was a problem hiding this comment.
I agree with Logan that using val is sufficient.
op-e2e/system_test.go
Outdated
| @@ -1496,24 +1498,49 @@ func TestChallenge(t *testing.T) { | |||
| } | |||
| cancel() | |||
|
|
|||
| // set a timeout for security council to validate output | |||
| ctx, cancel = context.WithTimeout(context.Background(), 10*time.Second) | |||
| findTransactionId := func() *big.Int { | |||
There was a problem hiding this comment.
I can understand why you named it findTransactionId since it's obvious what it does in the for loop underneath, but looking at that function alone, I don't understand what transaction you're trying to find or what output you want.
Also, using the auto-generated code in bindings would eliminate the need to reference the "ValidationRequested" field as a string.
Considering these two points, I would suggest removing findTransactionId and doing it this way.
securityCouncil, err := bindings.NewSecurityCouncil(cfg.L1Deployments.SecurityCouncilProxy, l1Client)if challengeStatus == chal.StatusNone {
// check validation request tx exists and not executed
toBlock := latestBlock(t, l1Client)
iter, _ := securityCouncil.FilterValidationRequested(&bind.FilterOpts{End: &toBlock}, nil)
iter.Next()
tx, err := securityCouncil.Transactions(&bind.CallOpts{}, iter.Event.TransactionId)
iter.Close()
....
}
op-e2e/system_test.go
Outdated
| defer cancel() | ||
|
|
||
| numCheck := 0 | ||
| for ; ; <-ticker.C { |
There was a problem hiding this comment.
even though the for loop is the last piece of code. This avoids referencing variables outside of the for loop.
for numCheck := 0; ; <-ticker.C {
op-e2e/system_test.go
Outdated
| @@ -1522,15 +1549,17 @@ func TestChallenge(t *testing.T) { | |||
| require.Equal(t, output.Submitter, cfg.Secrets.Addresses().Challenger1) | |||
| outputDeleted := val.IsOutputDeleted(output.OutputRoot) | |||
| require.True(t, outputDeleted) | |||
There was a problem hiding this comment.
I suppose it's a matter of taste, but I think the val.IsOutputDeleted function is so clear that you don't need to do local variable assignments.
require.True(t, val.IsOutputDeleted(output.OutputRoot))
seolaoh
force-pushed
the
fix/fix-challenge-e2e
branch
from
8907e09 to
ced1cf6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- kroma-chain-ops/genesis/layer_one.go (2 hunks)
- kroma-validator/guardian.go (8 hunks)
- op-e2e/actions/l2_challenger_test.go (1 hunks)
- op-e2e/actions/l2_guardian.go (1 hunks)
- op-e2e/actions/l2_runtime.go (1 hunks)
- op-e2e/e2eutils/geth/geth.go (1 hunks)
- op-e2e/system_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (6)
- kroma-validator/guardian.go
- op-e2e/actions/l2_challenger_test.go
- op-e2e/actions/l2_guardian.go
- op-e2e/actions/l2_runtime.go
- op-e2e/e2eutils/geth/geth.go
- op-e2e/system_test.go
Additional comments: 5
kroma-chain-ops/genesis/layer_one.go (5)
- 8-14: Imports have been added for several packages. Ensure that these new dependencies are necessary and correctly used within the file to avoid unnecessary bloat and potential conflicts.
- 111-118: Introduced a new functionality related to storage manipulation for the
KromaPortalcontract. It's crucial to ensure that thegetStorageSlotfunction is robust and handles errors appropriately, as it directly interacts with contract storage.Verification successful
The
getStorageSlotfunction inkroma-chain-ops/genesis/layer_one.gois implemented with robust error handling. It properly checks for errors at each critical step, such as when retrieving the storage layout for a contract and when fetching a specific entry within that layout. Errors are returned appropriately, ensuring that any issues encountered are communicated back to the caller. This approach allows for effective error management and contributes to the function's overall robustness in interacting with contract storage.* 120-132: The ownership transfer logic for `SystemConfig` to `ProxyAdminOwner` is updated. It's important to validate that the new ownership is correctly set and that this change does not introduce any unintended permissions or access control issues. * 134-153: The logic for modifying the storage key `_quorumNumeratorHistory` in `UpgradeGovernor` is complex and involves manipulating bytes directly. Ensure that this manipulation is accurate and does not introduce potential data corruption or unintended side effects. * 155-179: Updates to the keys of `_totalCheckpoints` in `SecurityCouncilToken` involve iterating over minted numbers and modifying storage slots. This operation seems to be critical and should be thoroughly tested to ensure it behaves as expected under all conditions, especially considering the direct manipulation of storage slots.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of getStorageSlot function for robustness and error handling rg "func getStorageSlot" --type goLength of output: 148
Script:
#!/bin/bash # Examine the implementation of getStorageSlot function in layer_one.go fd "layer_one.go" --exec cat {}Length of output: 7325
| func getStorageSlot(contractName, entryName string) (common.Hash, error) { | ||
| layout, err := bindings.GetStorageLayout(contractName) | ||
| if err != nil { | ||
| return common.Hash{}, fmt.Errorf("failed to get storage layout for %s", contractName) | ||
| } | ||
|
|
||
| entry, err := layout.GetStorageLayoutEntry(entryName) | ||
| if err != nil { | ||
| return common.Hash{}, fmt.Errorf("failed to get storage layout entry for %s.%s", contractName, entryName) | ||
| } | ||
|
|
||
| return common.BigToHash(big.NewInt(int64(entry.Slot))), nil | ||
| } |
There was a problem hiding this comment.
The getStorageSlot function is central to several storage manipulation operations. It's essential to ensure that this function is implemented correctly, especially error handling and the accuracy of slot calculations. Consider adding unit tests for this function to cover various scenarios and edge cases.
Would you like me to help with creating unit tests for the getStorageSlot function?
|
lgtm |
Description
Fixed below issues of challenge e2e tests.
kroma_getBlockTraceByNumberOrHash.Summary by CodeRabbit
Guardianstructure for clarity.