fix: add AllowUnprotectedTxs configuration option to JSON-RPC settings #1934
Conversation
XinyuCRO
requested review from
JayT106 and
randy-cro
and removed request for
a team
WalkthroughAdds JSON-RPC support for AllowUnprotectedTxs via a changelog entry and test config, updates the Ethermint module pin/hash, adds an integration test and fixture to verify unprotected tx replay, registers an evm pytest marker, and refactors test funding helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (1)
10-10: Consider calling out security implications in the changelog entry (opt-in; replay-risk tradeoff).
Since “allow unprotected txs” materially weakens replay protection guarantees, a short note that it’s opt-in and should be enabled only for specific compatibility/testing scenarios would help operators.integration_tests/utils.py (1)
861-870:func_addressfunding assertion is brittle; consider “top-up” semantics and>=checks.
Current behavior won’t top up partially-funded accounts and assumes post-state is exactlyfund. If these tests ever reuse addresses or run against environments that pre-fund accounts, it may flake.def func_address(w3, addr, fund=3000000000000000000): - if w3.eth.get_balance(addr, "latest") == 0: + bal = w3.eth.get_balance(addr, "latest") + if bal < fund: tx = {"to": addr, "value": fund, "gasPrice": w3.eth.gas_price} - send_transaction(w3, tx) - assert w3.eth.get_balance(addr, "latest") == fund + # send only the delta to reach `fund` + tx["value"] = fund - bal + send_transaction(w3, tx) + assert w3.eth.get_balance(addr, "latest") >= fundintegration_tests/test_basic.py (1)
995-1005: Fixed base port (27100) can cause test collisions in parallel runs; prefer dynamically allocated ports.
If CI shards/xdist runs modules in parallel, a hard-coded port is a common flake source. Consider deriving fromworker_id/env (or reuse existing port allocation helpers if present).
📜 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 (7)
CHANGELOG.md(1 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)integration_tests/configs/allow-unprotected-txs.jsonnet(1 hunks)integration_tests/conftest.py(1 hunks)integration_tests/test_basic.py(2 hunks)integration_tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_basic.py (2)
integration_tests/utils.py (1)
func_address(861-865)integration_tests/network.py (2)
setup_custom_cronos(155-197)w3(39-42)
⏰ 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: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: Run golangci-lint
🔇 Additional comments (5)
integration_tests/conftest.py (1)
13-28: LGTM: marker registration forevmis the right way to avoid pytest warnings.integration_tests/test_basic.py (1)
1007-1028: Nice coverage: validates unprotected-tx acceptance end-to-end; consider asserting the failure mode when disabled too.
You already havetest_replay_protectionfor the rejection case; this complements it well. Only suggestion: also assertreceipt["from"]/sender matches expected to avoid a false positive if the fixture file changes.gomod2nix.toml (1)
326-329: Nix pin and go.mod are in lockstep for ethermint. Both files reference the same version (v0.22.1-0.20251212064215-b7d7f71a9cc0) and replacement target (github.com/crypto-org-chain/ethermint). The pseudo-version and commit hashb7d7f71a9cc0are consistent.go.mod (1)
306-306: Ethermint bump migrates AllowUnprotectedTxs from global param to per-node RPC configuration.This dependency bump implements the intended feature: the allow_unprotected_txs parameter has moved from a global VM module setting to explicit per-node RPC configuration. By default, Ethermint rejects non-EIP-155 transactions; operators must explicitly enable the node RPC option to permit them. No unintended consensus or tx-validation changes were introduced—only the intentional parameter migration.
integration_tests/configs/allow-unprotected-txs.jsonnet (1)
1-29: Verify the json-rpc config key is actually consumed by Ethermint.The
allow-unprotected-txskey in app-config follows the jsonnet hyphenated naming convention (consistent with other json-rpc keys in default.jsonnet), and the genesis-levelallow_unprotected_txsfield correctly matches the proto definition. However, no references to this config key exist in the Cronos codebase itself—it's read upstream by Ethermint's JSON-RPC server initialization. Confirm with Ethermint thatallow-unprotected-txs(hyphenated) is the expected viper config key; if not, the effective config will silently ignore this setting despite the jsonnet merge succeeding.
thomas-nguy
left a comment
thomas-nguy
left a comment
There was a problem hiding this comment.
Perhaps add it to the release/v1.6.x branch as well to release it on testnet
ethermint PR: crypto-org-chain/ethermint#804
👮🏻👮🏻👮🏻 !!!! 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
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.