feat: backport release/v1.5.x to main #1906
Conversation
…to-org-chain#1892) * fix: disable memiavl cache when optimistic execution is enabled * update go.mod * fmt * add changelog
…in#1898) * check authorisation list for blocklisted address * v1.5.3 release * add changelog * fix lint
* cleanup release branch * cleanup
* fix prioritynoncemempool and add additional flags * add changelog * fix build * add tests * avoid conflicts * move tx replacement test to mempool * isort
* check authorisation list in e2ee * fix golang-ci * fix upgrade * add integration tests
thomas-nguy
requested review from
calvinaco and
songgaoye
and removed request for
a team
WalkthroughAdds EIP-7702 authorization checks to ante/proposal validation, introduces mempool fee-bump and tx-replacement flags/config and replacement logic, wires Cronos config/template, updates upgrade plan/version metadata and dependency pins, and adds/rewrites integration tests plus a new test contract. Changes
Sequence Diagram(s)sequenceDiagram
%% EIP-7702 authorization check flow
participant Client
participant Node as AnteHandler
participant MsgParser as EthereumMsgParser
participant AuthCheck as SetCodeAuthChecker
participant Blocklist
Client->>Node: Submit tx
Node->>MsgParser: Inspect messages for MsgEthereumTx
alt contains MsgEthereumTx
MsgParser->>AuthCheck: Extract SetCodeAuthorizations
AuthCheck->>Blocklist: Check each authority address
alt authority blocked
Blocklist-->>Node: Blocked address found
Node-->>Client: Reject (signer is blocked)
else allowed
Node-->>Client: Proceed (tx accepted)
end
else no Ethereum auth
Node-->>Client: Standard validation (tx accepted)
end
sequenceDiagram
%% Mempool fee bump replacement flow
participant Sender
participant Mempool as PriorityMempool
participant FeeCheck as FeeBumpPolicy
Sender->>Mempool: Submit replacement tx
Mempool->>FeeCheck: Compute required bump threshold (feeBump%)
FeeCheck-->>Mempool: Decision (allowed / rejected)
alt allowed
Mempool->>Mempool: Replace old tx with new tx
Mempool-->>Sender: Accepted
else rejected
Mempool-->>Sender: Error: tx doesn't fit replacement rule
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/proposal.go (1)
178-194: Code duplication with block_address.go.This EIP-7702 authorization check duplicates the logic in
app/block_address.go(lines 46-62). See the refactoring suggestion in that file's review comment.
🧹 Nitpick comments (3)
app/block_address.go (1)
46-62: Consider extracting the duplicated EIP-7702 authorization check.The authorization blocklist validation logic here is nearly identical to the implementation in
app/proposal.go(lines 177-194). Consider extracting this into a shared helper function to reduce duplication and improve maintainability.Example refactor:
// In a shared location (e.g., app/validation.go) func ValidateAuthorizationBlocklist(tx sdk.Tx, blocklist map[string]struct{}) error { for _, msg := range tx.GetMsgs() { msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) if ok { ethTx := msgEthTx.AsTransaction() if ethTx.SetCodeAuthorizations() != nil { for _, auth := range ethTx.SetCodeAuthorizations() { addr, err := auth.Authority() if err == nil { if _, ok := blocklist[sdk.AccAddress(addr.Bytes()).String()]; ok { return fmt.Errorf("signer is blocked: %s", addr.String()) } } } } } } return nil }Then use it in both locations:
if err := ValidateAuthorizationBlocklist(tx, bad.blockedMap); err != nil { return ctx, err }app/app.go (1)
429-432: Consider clarifying the cache enabling logic.The condition
!blockSTMEnabled && optimisticExecutionDisabledis technically correct but uses double negatives that may confuse future maintainers. Consider refactoring for clarity:- if !blockSTMEnabled && optimisticExecutionDisabled { - // only enable memiavl cache if neither block-stm nor optimistic execution is enabled, because it's not concurrency-safe. + // Enable memiavl cache only when both block-stm and optimistic execution are disabled, + // because the cache is not concurrency-safe. + if !blockSTMEnabled && !cast.ToBool(appOpts.Get(FlagDisableOptimisticExecution)) == false {Or more explicitly:
+ optimisticExecutionEnabled := !optimisticExecutionDisabled + if !blockSTMEnabled && !optimisticExecutionEnabled { + // Enable memiavl cache only when both block-stm and optimistic execution are disabledcmd/cronosd/cmd/root.go (1)
17-17: Consider a more descriptive import alias.The alias
config2is functional but lacks clarity. Consider using a more descriptive name likecronosconfigorcmdconfigto improve code readability.Apply this diff to use a more descriptive alias:
- config2 "github.com/crypto-org-chain/cronos/cmd/cronosd/config" + cronosconfig "github.com/crypto-org-chain/cronos/cmd/cronosd/config"Then update references at lines 276, 285, and 288:
- Cronos config2.CronosConfig `mapstructure:"cronos"` + Cronos cronosconfig.CronosConfig `mapstructure:"cronos"`- Cronos: config2.DefaultCronosConfig(), + Cronos: cronosconfig.DefaultCronosConfig(),- return tpl + memiavlcfg.DefaultConfigTemplate + DefaultVersionDBTemplate + config2.DefaultCronosConfigTemplate, customAppConfig + return tpl + memiavlcfg.DefaultConfigTemplate + DefaultVersionDBTemplate + cronosconfig.DefaultCronosConfigTemplate, customAppConfig
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)app/app.go(5 hunks)app/block_address.go(2 hunks)app/proposal.go(2 hunks)app/upgrades.go(1 hunks)cmd/cronosd/cmd/root.go(3 hunks)cmd/cronosd/config/config.go(1 hunks)cmd/cronosd/config/toml.go(1 hunks)default.nix(1 hunks)integration_tests/configs/default.jsonnet(1 hunks)integration_tests/configs/upgrade-test-package.nix(2 hunks)integration_tests/test_basic.py(1 hunks)integration_tests/test_e2ee.py(2 hunks)integration_tests/test_mempool.py(2 hunks)integration_tests/test_upgrade.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
integration_tests/test_basic.py (1)
integration_tests/utils.py (2)
deploy_contract(421-441)send_transaction(466-469)
integration_tests/test_upgrade.py (2)
integration_tests/cosmoscli.py (1)
block_height(177-178)integration_tests/network.py (1)
w3(39-42)
app/block_address.go (1)
testground/benchmark/benchmark/cosmostx.py (1)
MsgEthereumTx(80-86)
app/app.go (1)
store/setup.go (2)
FlagCacheSize(23-23)SetupMemIAVL(29-59)
app/proposal.go (1)
testground/benchmark/benchmark/cosmostx.py (1)
MsgEthereumTx(80-86)
cmd/cronosd/cmd/root.go (2)
cmd/cronosd/config/config.go (2)
CronosConfig(12-17)DefaultCronosConfig(19-24)cmd/cronosd/config/toml.go (1)
DefaultCronosConfigTemplate(4-16)
integration_tests/test_mempool.py (2)
integration_tests/utils.py (4)
get_account_nonce(461-463)replace_transaction(472-476)send_transaction(466-469)wait_for_new_blocks(133-141)integration_tests/network.py (3)
w3(39-42)cosmos_cli(58-59)cosmos_cli(83-84)
integration_tests/test_e2ee.py (3)
integration_tests/utils.py (3)
derive_new_account(485-489)fund_acc(860-865)wait_for_new_blocks(133-141)integration_tests/network.py (1)
w3(39-42)integration_tests/cosmoscli.py (1)
address(302-312)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
11-11: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: integration_tests (gas)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (23)
integration_tests/test_basic.py (1)
1084-1100: LGTM!The test setup follows established patterns for contract deployment and transaction sending. The function is appropriately renamed to reflect its new purpose.
CHANGELOG.md (1)
11-19: Verify the future release date.The v1.5.4 release is dated Nov 30, 2025, which is 19 days in the future from the current date (November 11, 2025). Please confirm this is intentional—typically, changelog entries are added after a release, not before.
default.nix (1)
14-14: LGTM!The version bump from v1.5.0 to v1.6.0 aligns correctly with the broader v1.6 upgrade plan described in the PR.
integration_tests/configs/upgrade-test-package.nix (1)
27-29: LGTM!The upgrade test configuration correctly adds v1.5.4 as an intermediate step before v1.6. The structure follows the established pattern for version progression.
Also applies to: 54-60
cmd/cronosd/config/toml.go (1)
1-16: LGTM!The TOML template is well-structured and clearly documented. The two configuration flags (disable-tx-replacement and disable-optimistic-execution) are properly templated and will integrate cleanly with the application configuration system.
cmd/cronosd/config/config.go (1)
12-24: LGTM!The
CronosConfigstruct and default constructor are clean and well-documented. The default values (bothfalse) sensibly enable both transaction replacement and optimistic execution by default, which aligns with optimal performance settings for most nodes.app/app.go (2)
385-405: LGTM - Fee bump implementation looks correct.The priority mempool configuration with fee-based replacement is well-implemented. The arithmetic
np >= op*threshold/100correctly enforces that the new transaction priority must be at least(100 + feeBump)%of the old priority.One minor observation: with very large priority values and fee bump percentages, the multiplication
op*thresholdcould theoretically overflow, but given realistic values (priorities in int64 range, feeBump typically < 100), this is not a practical concern.
985-1000: LGTM - Tx replacement configuration is correct.The logic correctly disables transaction replacement by setting
mempoolCacheMaxTxs = -1when the disable flag is set. The logging clearly indicates whether the feature is enabled or disabled.cmd/cronosd/cmd/root.go (3)
271-277: LGTM!The
Cronosfield is properly integrated intoCustomAppConfig, following the same pattern as the existingMemIAVLandVersionDBfields. The mapstructure tag is correctly set.
281-286: LGTM!The Cronos configuration is properly initialized using
DefaultCronosConfig(), maintaining consistency with other configuration fields.
288-288: LGTM!The template concatenation properly includes the Cronos configuration template, following the established pattern.
integration_tests/configs/default.jsonnet (1)
29-32: LGTM!The
feebumpsetting (value 10) is correctly placed within the mempool configuration. This 10% fee bump threshold aligns with the transaction replacement logic tested intest_mempool.py.integration_tests/test_upgrade.py (2)
171-171: LGTM!Adding "v1.6" to the upgrade path condition is correct and aligns with the upgrade plan change in
app/upgrades.go.
340-344: LGTM!The v1.6 upgrade step is correctly added, following the same pattern as the v1.5 upgrade. The repeated transaction validation ensures backward compatibility is maintained after the upgrade.
integration_tests/test_mempool.py (4)
12-14: LGTM!The new imports are correctly added to support the transaction replacement test functionality.
146-174: LGTM!The test correctly validates that transaction replacement with a fee bump less than 10% is rejected with the expected error message.
176-202: LGTM!The test correctly validates that transaction replacement with a 15% fee bump succeeds, and properly verifies that the replacement transaction (value=5) was mined instead of the original (value=3).
204-228: LGTM!The test correctly validates that already mined transactions cannot be replaced, with proper error message verification.
integration_tests/test_e2ee.py (4)
9-16: LGTM!The new imports are correctly added to support the EIP-7702 blocklist test functionality.
162-171: LGTM!The test setup correctly creates and funds a new account, then applies the blocklist targeting that account.
173-203: LGTM!The EIP-7702 authorization transaction is correctly constructed with type 4, signed authorization, and proper validation that it enters the mempool when blocked.
205-211: LGTM!The test correctly validates that clearing the blocklist allows the previously blocked transaction to be mined, with proper verification of nonce increment and empty mempool filter.
app/upgrades.go (1)
14-18: Review appears valid but store deletion removal is intentional.Verification shows "capability" and "feeibc" modules are no longer referenced in the application—neither module is initialized in the ModuleManager (app/app.go lines 770-813), and "feeibc" has zero references in the codebase. The "capability" string appears only in documentation comments within e2ee and cronos modules.
The removal of store deletion logic is consistent with this: the modules were already removed, so their cleanup code is no longer needed. The simplified upgrade handler (returning
falseand only runningRunMigrations) is appropriate.Recommendation: Update the "TBD" comment on line 14 to clarify the upgrade purpose (e.g., "v1.6 - module migrations only").
| assert data["value"] == 0 | ||
| assert data["data"] == bytes.fromhex("8e091b5e") | ||
| assert data["price"] > 0 | ||
| assert data["gas"] == 3633 |
There was a problem hiding this comment.
Avoid hardcoding exact gas values.
The assertion assert data["gas"] == 3633 is fragile and may break with compiler optimizations, EVM gas schedule updates, or contract modifications. Consider using a range check or verifying that gas is within expected bounds instead.
Apply this diff to make the assertion more resilient:
- assert data["gas"] == 3633
+ assert data["gas"] > 0 and data["gas"] < 10000, f"unexpected gas: {data['gas']}"Alternatively, if the exact value is critical for detecting regressions, add a comment explaining why this specific value is expected and should be monitored closely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert data["gas"] == 3633 | |
| assert data["gas"] > 0 and data["gas"] < 10000, f"unexpected gas: {data['gas']}" |
🤖 Prompt for AI Agents
In integration_tests/test_basic.py around line 1124, the test currently asserts
an exact gas value with `assert data["gas"] == 3633`, which is brittle; change
this to a resilient check such as asserting the gas is within an acceptable
range (e.g., lower_bound <= data["gas"] <= upper_bound) or use a relative
tolerance (e.g., data["gas"] <= expected_max or abs(data["gas"] - expected) <=
tolerance), or if the exact value is critical leave the equality but add a clear
comment explaining why 3633 must be exact and should be monitored for
regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_tests/test_e2ee.py (1)
173-173: Consider adding a comment for the hardcoded target address.The hardcoded
target_addressvalue could benefit from a brief comment explaining its purpose in the EIP-7702 authorization test.Apply this diff to add clarification:
+ # Target address for EIP-7702 delegation target_address = "0x4Cd241E8d1510e30b2076397afc7508Ae59C66c9"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_e2ee.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_e2ee.py (3)
integration_tests/utils.py (3)
derive_new_account(485-489)fund_acc(860-865)wait_for_new_blocks(133-141)integration_tests/network.py (1)
w3(39-42)integration_tests/cosmoscli.py (1)
address(302-312)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
integration_tests/test_e2ee.py (2)
9-16: LGTM!The new imports
derive_new_accountandfund_accare properly utilized in the newtest_block_list_eip7702test function.
162-162: Past issue resolved.The function name typo flagged in previous reviews (
test_block_list_eip7022→test_block_list_eip7702) has been corrected. The function is now properly named to match the EIP-7702 authorization being tested.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1906 +/- ##
===========================================
+ Coverage 16.87% 37.19% +20.31%
===========================================
Files 72 127 +55
Lines 6163 9741 +3578
===========================================
+ Hits 1040 3623 +2583
- Misses 5000 5697 +697
- Partials 123 421 +298
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integration_tests/contracts/contracts/TestBlockTxProperties.sol (1)
14-16: Consider the non-deterministic nature ofgasleft().The
gasleft()function returns the remaining gas at the point of the call, which varies based on the initial gas limit, gas consumed prior to this point, and EVM implementation details. When asserting against the emittedgasvalue in tests, ensure the test accounts for this variability or validates it within an acceptable range rather than expecting an exact value.integration_tests/test_e2ee.py (3)
167-167: Consider using a unique account index or documenting the choice.The hardcoded account index
n=12could potentially conflict with other tests if they use the same derivation index. Consider using a dynamically generated unique index or documenting why this specific index was chosen.Apply this approach to reduce test coupling:
# Use a higher index or make it test-specific acc = derive_new_account(n=100) # Higher index reduces collision risk
173-173: Document the purpose of the target address.The
target_addressvalue0x4Cd241E8d1510e30b2076397afc7508Ae59C66c9is hardcoded without explanation. Adding a comment clarifying what this address represents (e.g., a specific contract, test account, or arbitrary value) would improve test readability.# Example documentation target_address = "0x4Cd241E8d1510e30b2076397afc7508Ae59C66c9" # Arbitrary contract address for testing authorization
162-210: Consider verifying that EIP-7702 authorization actually succeeded.The test verifies that the transaction was included in the mempool and the nonce incremented, but doesn't validate that the EIP-7702 authorization actually worked (e.g., checking that code delegation occurred at
acc.address). Consider adding an assertion to query the account's delegated code after transaction execution to ensure the authorization mechanism is functioning correctly.# After line 209, verify the authorization worked delegated_code = w3.eth.get_code(acc.address) assert delegated_code != b"", "Authorization should have delegated code to the account"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/cronosd/cmd/root.go(3 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/contracts/TestBlockTxProperties.sol(1 hunks)integration_tests/test_e2ee.py(2 hunks)integration_tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cronosd/cmd/root.go
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_e2ee.py (3)
integration_tests/utils.py (3)
derive_new_account(486-490)fund_acc(861-866)wait_for_new_blocks(134-142)integration_tests/network.py (1)
w3(39-42)integration_tests/cosmoscli.py (1)
address(302-312)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Run golangci-lint
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
gomod2nix.toml (1)
314-317: Dependency version update consistent with go.mod.The ethermint version bump is properly reflected in both the Nix lock file and the Go module file. Both reference v0.22.1-0.20251031062550-bd820ee1e663 and maintain the same replacement target (github.com/crypto-org-chain/ethermint).
go.mod (1)
306-306: Dependency version update consistent with gomod2nix.toml.The ethermint replace directive has been updated to v0.22.1-0.20251031062550-bd820ee1e663, consistent with the corresponding update in gomod2nix.toml. The replacement target remains stable at github.com/crypto-org-chain/ethermint.
integration_tests/utils.py (1)
68-68: LGTM! Test contract mapping added correctly.The mapping entry for
TestBlockTxPropertiesis consistent with the existing contract mappings and enables test artifact resolution for the new contract.integration_tests/test_e2ee.py (1)
9-16: LGTM! Required imports added for the new test.The additional imports (
derive_new_account,fund_acc,wait_for_new_blocks,wait_for_port) are all utilized in the newtest_block_list_eip7702function.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
* add integration tests for multiple replace txs * change wait block * fix * change dep to crypto org chain
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (1)
11-11: Minor: Date formatting uses emphasis instead of markdown headings.Lines 11, 22, 35, and throughout the file use
*Date*format (markdown emphasis), which linters flag as MD036 (no-emphasis-as-heading). However, this is the established convention throughout the entire changelog file, so this is consistent with the existing style rather than a regression.If desired in a future pass, consider standardizing dates to a different visual format (e.g.,
**Date**for bold, or plain text) to align with markdown best practices.Also applies to: 22-22, 35-35, 43-43
integration_tests/test_mempool.py (1)
146-228: Tx replacement test covers key paths; tighten exception checks and namingThe overall structure nicely validates:
- insufficient fee bump is rejected,
- a >10% bump replaces and mines the new tx,
- and a mined tx cannot be replaced.
Two small robustness/readability tweaks:
- In both
pytest.raisesblocks, assert onexc.valuerather than theExceptionInfowrapper:- assert "tx doesn't fit the replacement rule" in str(exc) + assert "tx doesn't fit the replacement rule" in str(exc.value) ... - assert "has already been mined" in str(exc) + assert "has already been mined" in str(exc.value)
- Consider fixing the typo in
txhash_noreplacemenetto something liketxhash_no_replacementfor clarity.Also, this test calls
cronos_mempool.cosmos_cli()without an index (line 176), whereas earlier tests in this file usecosmos_cli(0). Please confirm the default index is what you expect, or align the call style for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)go.mod(2 hunks)gomod2nix.toml(2 hunks)integration_tests/test_mempool.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_mempool.py (1)
integration_tests/utils.py (5)
get_account_nonce(462-464)replace_transaction(473-477)send_transaction(467-470)wait_for_new_blocks(134-142)sign_transaction(453-459)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
11-11: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
gomod2nix.toml (2)
216-217: Verify cosmos-sdk dependency version and hash.The cosmos-sdk version was updated with a newer timestamp (20251114, which is after the PR creation date of 20251111). Ensure this version bump is intentional and that the newer commits in the dependency are compatible with this rebase.
Consider running a build and integration tests to validate that this dependency upgrade does not introduce breaking changes or compatibility issues.
315-316: Verify ethermint dependency version and hash.The ethermint version was updated with a newer timestamp (20251112, which is after the PR creation date of 20251111). Ensure this version bump is intentional and that the newer commits in the dependency are compatible with this rebase.
Confirm that both the cosmos-sdk and ethermint updates have been tested together in the rebase context.
CHANGELOG.md (1)
5-7: Changelog entries are well-structured and accurate, with proper content preservation.The removal of #1869 and #1872 from UNRELEASED and insertion of the v1.5.4, v1.5.3, v1.5.2, and v1.5.1 release sections maintains chronological order and properly documents the recent release history. All PR references are correctly formatted and linked.
Also applies to: 11-20, 22-40, 43-43
integration_tests/test_mempool.py (2)
7-18: New helper imports are consistent and all used
get_account_nonce,replace_transaction, andsend_transactionline up with the utilities shown inintegration_tests/utils.pyand are all exercised by the new tests, so the import changes look good.
231-308: Multi-account replacement scenario is well covered; verify cosmos_cli usageThis test gives good coverage of concurrent replacement behavior across multiple nonces/accounts and correctly asserts that only the highest-fee transactions contribute to the final community balance (
1→2→3,10→11,20→ total34).One thing to double-check: here
cli = cronos_mempool.cosmos_cli()(no index), while earlier tests in the same module callcosmos_cli(0). Ifcosmos_clidoesn’t have a default or if the default could change, it’s safer to be explicit and align with the existing pattern.
thomas-nguy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integration_tests/test_mempool.py (4)
152-177: Consider verifying the original transaction status after failed replacement.After the failed replacement attempt (lines 152-174), the original transaction (value 1) remains in the mempool. When blocks are waited on line 176, that transaction should be mined. Consider explicitly verifying that:
- The original transaction was mined
- The nonce was incremented as expected
This would make the test's intent clearer and catch potential issues where the original transaction doesn't get mined.
Example verification:
assert "fit the replacement rule" in str(exc) wait_for_new_blocks(cronos_mempool.cosmos_cli(), 1) + # Verify the original transaction was mined + assert w3.eth.get_transaction_count(ADDRS["validator"]) == nonce + 1 nonce = get_account_nonce(w3)
200-201: Potentially flaky assertion on transaction index.Asserting
transactionIndex == 0assumes this is the first transaction in the block, which may not always be true in a concurrent environment. While the test is already marked as flaky, consider whether this assertion is necessary for the test's purpose or if it could be removed to reduce flakiness.
174-174: Error message assertions are brittle.The test relies on exact substring matches in error messages ("fit the replacement rule" on line 174 and "has already been mined" on line 228). If the error message format changes in the underlying library or node implementation, these tests will break. Consider whether these specific message checks are necessary or if catching the exception type is sufficient.
Also applies to: 228-228
301-308: Consider verifying that only final replacement transactions were mined.The test verifies the final balance is correct (initial + 34), which indirectly confirms the replacement logic worked. However, for more robust verification, consider explicitly checking that:
- Only the final version of each replaced transaction was mined (tx1_replaced_again, tx2_replaced, tx3)
- The earlier versions (tx1, tx1_replaced, tx2) were NOT mined
This would make the test more explicit about what it's validating and could catch subtle bugs where multiple versions of the same transaction get included.
Example additional verification:
# After line 308, add verification that only final txs were mined mined_txs = w3.eth.get_block("latest", full_transactions=True)["transactions"] mined_values = [tx["value"] for tx in mined_txs if tx["to"] == ADDRS["community"]] assert set(mined_values) == {3, 11, 20}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_mempool.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_mempool.py (1)
integration_tests/utils.py (5)
get_account_nonce(462-464)replace_transaction(473-477)send_transaction(467-470)wait_for_new_blocks(134-142)sign_transaction(453-459)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
integration_tests/test_mempool.py (1)
4-4: LGTM: Import additions are clean and necessary.The new imports for Web3 exceptions and transaction utilities are properly used in the new test functions below.
Also applies to: 12-14
2 participants
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Chores
Tests