fix: emit events on revert #2466
Conversation
WalkthroughThe recent updates primarily address changing assertions in test cases to compare with zero values instead of Changes
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2466 +/- ##
============================================
- Coverage 69.05% 47.19% -21.86%
============================================
Files 308 444 +136
Lines 19192 29372 +10180
============================================
+ Hits 13253 13863 +610
- Misses 5264 14704 +9440
- Partials 675 805 +130
|
|
It seems like I have some issue to retrieve the tx with Cast CLI ReproduceRun localnet Run deposit failure Get the synthetic tx hash from the cctx Query tx data Wondering if this could be due to cast specifically? There is something queried at least, it doesn't show tx not found |
i think this should be fixed with 4581257 |
skosito
requested review from
brewmaster012,
ws4charlie,
swift1337 and
fbac
as code owners
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- rpc/backend/blocks_test.go (1 hunks)
- rpc/backend/tx_info_test.go (3 hunks)
- rpc/types/utils.go (1 hunks)
- x/crosschain/keeper/cctx_gateway_zevm.go (1 hunks)
- x/fungible/keeper/evm.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/fungible/keeper/evm.go
Additional context used
Path-based instructions (4)
x/crosschain/keeper/cctx_gateway_zevm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/types/utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/backend/tx_info_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/backend/blocks_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
rpc/types/utils.go
[warning] 259-261: rpc/types/utils.go#L259-L261
Added lines #L259 - L261 were not covered by tests
Additional comments not posted (5)
x/crosschain/keeper/cctx_gateway_zevm.go (1)
36-36: LGTM! Ensure correct handling of all statuses.The change ensures that transactions are committed unless the new status is
CctxStatus_Aborted, which aligns with the goal of emitting events on revert.However, verify that all statuses are correctly handled and there are no unintended side effects.
Verification successful
LGTM! Ensure correct handling of all statuses.
The change ensures that transactions are committed unless the new status is
CctxStatus_Aborted, which aligns with the goal of emitting events on revert. The extensive tests confirm that all statuses are correctly handled, and there are no unintended side effects.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of all statuses in the `InitiateOutbound` function. # Test: Search for all usages of `InitiateOutbound`. Expect: Proper handling of all statuses. rg --type go -A 10 'InitiateOutbound'Length of output: 27692
rpc/backend/tx_info_test.go (3)
62-64: LGTM!The changes ensure that the
V,R, andSfields are compared to(*hexutil.Big)(big.NewInt(0))instead of being checked fornil, aligning with the updated default values.
130-132: LGTM!The changes ensure that the
V,R, andSfields are compared to(*hexutil.Big)(big.NewInt(0))instead of being checked fornil, aligning with the updated default values.
163-165: LGTM!The changes ensure that the
V,R, andSfields are compared to(*hexutil.Big)(big.NewInt(0))instead of being checked fornil, aligning with the updated default values.rpc/backend/blocks_test.go (1)
1719-1721: LGTM! Ensure consistency across the codebase.The changes to compare
V,R, andSfields to(*hexutil.Big)(big.NewInt(0))instead ofnilare approved.However, ensure that similar changes are consistently applied across the codebase.
Verification successful
Changes are consistently applied across the codebase.
The new assertions for
V,R, andSfields are present inrpc/backend/tx_info_test.go, confirming consistency with the changes inrpc/backend/blocks_test.go.
rpc/backend/tx_info_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all assertions for `V`, `R`, and `S` fields are consistent across the codebase. # Test: Search for the assertions. Expect: Only occurances of the new assertions. rg --type go $'.Equal((*hexutil.Big)(big.NewInt(0)), res.V)' rg --type go $'.Equal((*hexutil.Big)(big.NewInt(0)), res.R)' rg --type go $'.Equal((*hexutil.Big)(big.NewInt(0)), res.S)'Length of output: 569
Script:
#!/bin/bash # Description: Verify that all assertions for `V`, `R`, and `S` fields are consistent across the codebase. # Test: Search for the assertions. Expect: Only occurrences of the new assertions. rg --type go '\.Equal\(\(\*hexutil\.Big\)\(big\.NewInt\(0\)\), res\.V\)' rg --type go '\.Equal\(\(\*hexutil\.Big\)\(big\.NewInt\(0\)\), res\.R\)' rg --type go '\.Equal\(\(\*hexutil\.Big\)\(big\.NewInt\(0\)\), res\.S\)'Length of output: 1025
There was a problem hiding this comment.
It looks good to me.
FYI I pushed an additional assertion for ZEVM MP test, I wanted to ensure the commit refactoring didn't allow to mint ZETA into fungible module on revert.
It looks the change doesn't affect the system as the deposit of token and call are atomic into the EVM call
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
cmd/zetae2e/config/local.yml (1)
34-34: Add a newline at the end of the file.The file should end with a newline character to adhere to the POSIX standard.
+Tools
yamllint
[error] 34-34: no new line character at the end of file
(new-line-at-end-of-file)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- cmd/zetae2e/config/local.yml (1 hunks)
- e2e/e2etests/test_message_passing_evm_to_zevm_revert.go (3 hunks)
Additional context used
Path-based instructions (1)
e2e/e2etests/test_message_passing_evm_to_zevm_revert.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
yamllint
cmd/zetae2e/config/local.yml
[error] 34-34: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (3)
cmd/zetae2e/config/local.yml (1)
34-34: Verify the newtest_dappaddress.Ensure that the new
test_dappaddress under theevmsection is correct and consistent with the rest of the configuration.Verification successful
The new
test_dappaddress is correctly placed and consistent with the rest of the configuration.
- The address
0xBFF76e77D56B3C1202107f059425D56f0AEF87Edis correctly located under theevmsection in thelocal.ymlfile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new test_dapp address in the configuration file. # Test: Search for the new test_dapp address in the codebase. Expect: Only valid occurrences. rg --type yaml '0xBFF76e77D56B3C1202107f059425D56f0AEF87Ed'Length of output: 148
Script:
#!/bin/bash # Description: Search for the evm section and check the context around the test_dapp entry in the local.yml file. # Find the evm section and display surrounding lines to verify the context of test_dapp rg --type yaml -A 10 'evm' cmd/zetae2e/config/local.ymlLength of output: 1357
Tools
yamllint
[error] 34-34: no new line character at the end of file
(new-line-at-end-of-file)
e2e/e2etests/test_message_passing_evm_to_zevm_revert.go (2)
55-57: LGTM! Ensure balance checks are accurate.The balance checks for the fungible module address enhance the test coverage. Ensure these checks are accurate and consistent with the expected behavior.
Also applies to: 118-129
16-17: Verify the new constantfungibleModuleAddress.Ensure that the new constant
fungibleModuleAddressis correct and used appropriately in the test.Verification successful
The new constant
fungibleModuleAddressis correctly defined and used appropriately in the test.
- The constant is defined and used within the
TestMessagePassingEVMtoZEVMRevertfunction.- It is converted to an Ethereum address and validated, ensuring its correctness.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new constant fungibleModuleAddress in the codebase. # Test: Search for the new constant in the codebase. Expect: Only valid occurrences. rg --type go 'const fungibleModuleAddress = "0x735b14BB79463307AAcBED86DAf3322B1e6226aB"'Length of output: 222
Script:
#!/bin/bash # Description: Verify the usage of the constant fungibleModuleAddress in the test file. # Test: Search for all occurrences of fungibleModuleAddress in the test file and include context lines. rg 'fungibleModuleAddress' -A 5 -B 5 e2e/e2etests/test_message_passing_evm_to_zevm_revert.goLength of output: 764
thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- x/crosschain/keeper/cctx_gateway_zevm.go (1 hunks)
- x/fungible/keeper/evm.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/crosschain/keeper/cctx_gateway_zevm.go
- x/fungible/keeper/evm.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/crosschain/keeper/cctx_gateway_zevm.go (1)
36-36: Clarify the commit logic in relation to transaction statuses.The current implementation commits the transaction if the new status is either
OutboundMinedorPendingRevert. This change aligns with the previous discussion about ensuring that reverted transactions are still committed to allow for event emission and indexing. However, it would be beneficial to add a comment explaining this decision for future maintainability.It's also important to ensure that all possible statuses where a commit is necessary are covered. The inclusion of
PendingRevertis a significant change that should be explicitly documented in the code to avoid confusion.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- x/crosschain/keeper/cctx_gateway_zevm.go (1 hunks)
Additional context used
Path-based instructions (1)
x/crosschain/keeper/cctx_gateway_zevm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Description
in progress
How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
V,R, andSfields to0instead ofnil.New Features
test_dappin thelocal.ymlfile with a specified Ethereum address.Documentation