Problem: mixed mempool setup is not tested#106
Conversation
WalkthroughIntroduces a new integration test for mixed mempool configurations where validators use nop mempool and fullnodes use eVM mempool. Adds a Jsonnet configuration file, a new test suite validating mempool behavior with statesync, and refactored utilities supporting concurrent contract deployments and flexible app configuration. Changes
Sequence DiagramsequenceDiagram
participant Test as test_mix_async
participant CLI as Cosmos CLI
participant Node as Mantra Node
participant RPC as EVM RPC
participant Web3 as AsyncWeb3
participant Deploy as deploy_multi_contracts
Test->>CLI: Initialize cosmos CLI with custom_mantra config
Test->>Node: Create node with statesync enabled
Test->>Node: Update app config (mempool.max-txs=5000)
Test->>Node: Start node process
Test->>Node: Advance blockchain by 1+ block
Test->>Node: Verify not catching up (get_sync_info)
Test->>RPC: Wait for EVM RPC port available
Test->>Web3: Construct AsyncWeb3 client
Test->>Deploy: Deploy contracts via Web3
Deploy->>Web3: Concurrent ERC20 deployments
Deploy->>Web3: Mint tokens and verify balances
Deploy-->>Test: Return deployment results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Heterogeneous changes across configuration, utilities, and tests require separate reasoning for each component. The new async orchestration logic in utils and cluster setup logic in the test file introduce complexity, offset by straightforward refactoring in test_contract.py and reuse of existing infrastructure patterns. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| exception_retry_configuration=RETRY_CONFIG, | ||
| ), | ||
| ) | ||
| await deploy_multi_contracts(w3, num=1) |
There was a problem hiding this comment.
enable evm mempool in fullnode can't avoid tx nonce is higher error, only can validator enable evm mempool
![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
🧹 Nitpick comments (3)
integration_tests/configs/mix_mempool.jsonnet (1)
2-2: Unused variable ‘chain’.Either use it (e.g., to source validators or derive the network key) or drop the binding.
integration_tests/utils.py (1)
1058-1080: Harden deploy_multi_contracts: assert all receipt statuses.Currently only exceptions are checked. Also assert receipts succeeded to catch on-chain reverts.
Apply this diff:
receipts = await asyncio.gather( *(send_transaction_async(w3, acct, **tx) for tx in txs), return_exceptions=True ) for r in receipts: if isinstance(r, Exception): pytest.fail(f"send_transaction failed: {r}") + assert r.get("status") == 1, f"deployment tx failed: {r}" assert len(receipts) == numintegration_tests/test_mix_mempool.py (1)
38-42: Mixed mempool intent not fully expressed — set mempool type explicitly.Objective says validators use nop mempool and full nodes use evm. Here only max-txs is set. Recommend setting type for the statesync node to evm and ensuring validators are configured to nop (in mix_mempool.jsonnet).
Apply this diff locally:
edit_app_cfg( clustercli, i, - app_config={"mempool": {"max-txs": 5000}}, + app_config={"mempool": {"type": "evm", "max-txs": 5000}}, )Also verify validators are configured with mempool.type = "nop" in integration_tests/configs/mix_mempool.jsonnet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
integration_tests/configs/mix_mempool.jsonnet(1 hunks)integration_tests/test_contract.py(2 hunks)integration_tests/test_mix_mempool.py(1 hunks)integration_tests/utils.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
integration_tests/utils.py
1034-1034: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (2)
integration_tests/test_contract.py (1)
321-322: LGTM: simplified multi-deploy flow via helper.Delegating to deploy_multi_contracts(w3) reduces duplication and keeps tests focused.
integration_tests/utils.py (1)
23-24: jsonmerge is already available as a transitive dependency — no ImportError risk.The import will not fail in CI.
jsonmergeis present inintegration_tests/poetry.lock(v1.9.2) and will be installed when dependencies are resolved, so there's no risk ofImportError. While best practice suggests explicitly declaring all direct runtime dependencies inpyproject.tomlrather than relying on transitive dependencies, the stated concern in this comment is unfounded.Likely an incorrect or invalid review comment.
| validators: [validator { | ||
| 'coin-type':: validator['coin-type'], | ||
| 'app-config'+: { | ||
| mempool: { | ||
| 'max-txs': -1, | ||
| }, | ||
| }, | ||
| } for validator in super.validators], |
There was a problem hiding this comment.
Potential Jsonnet inheritance bug: super.validators may be undefined; remove hidden field and add fallback.
If the base config lacks 'mantra-canary-net-1', using super.validators will fail. Also, defining 'coin-type' with :: creates a hidden field and is unnecessary here. Use chain.validators as a fallback and drop the hidden field.
Apply this diff:
local config = import 'default.jsonnet';
local chain = (import 'chains.jsonnet')[std.extVar('CHAIN_CONFIG')];
config {
'mantra-canary-net-1'+: {
- validators: [validator {
- 'coin-type':: validator['coin-type'],
- 'app-config'+: {
- mempool: {
- 'max-txs': -1,
- },
- },
- } for validator in super.validators],
+ validators: [
+ v {
+ 'app-config'+: {
+ mempool: {
+ 'max-txs': -1,
+ },
+ },
+ }
+ for v in (
+ if std.objectHas(super, 'validators') then super.validators else chain.validators
+ )
+ ],
},
}📝 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.
| validators: [validator { | |
| 'coin-type':: validator['coin-type'], | |
| 'app-config'+: { | |
| mempool: { | |
| 'max-txs': -1, | |
| }, | |
| }, | |
| } for validator in super.validators], | |
| validators: [ | |
| v { | |
| 'app-config'+: { | |
| mempool: { | |
| 'max-txs': -1, | |
| }, | |
| }, | |
| } | |
| for v in ( | |
| if std.objectHas(super, 'validators') then super.validators else chain.validators | |
| ) | |
| ], |
🤖 Prompt for AI Agents
In integration_tests/configs/mix_mempool.jsonnet around lines 6–13, avoid
referencing super.validators directly (it may be undefined) and remove the
hidden-field use of 'coin-type'::; instead iterate over a fallback like
(super.validators // chain.validators) and reference coin-type as a normal field
(validator["coin-type"] or validator['coin-type']) without the :: operator,
keeping the same app-config+ mempool override; update the list comprehension to
use the fallback and drop the hidden-field syntax.
| def edit_app_cfg(cli, i, app_config={}): | ||
| # Modify the json-rpc addresses to avoid conflict | ||
| cluster.edit_app_cfg( | ||
| cli.home(i) / "config/app.toml", | ||
| cli.base_port(i), | ||
| { | ||
| "json-rpc": { | ||
| "enable": True, | ||
| "address": "127.0.0.1:{EVMRPC_PORT}", | ||
| "ws-address": "127.0.0.1:{EVMRPC_PORT_WS}", | ||
| jsonmerge.merge( | ||
| { | ||
| "json-rpc": { | ||
| "enable": True, | ||
| "address": "127.0.0.1:{EVMRPC_PORT}", | ||
| "ws-address": "127.0.0.1:{EVMRPC_PORT_WS}", | ||
| }, | ||
| }, | ||
| }, | ||
| app_config, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix mutable default arg in edit_app_cfg (B006).
Using {} as a default can leak state across calls. Use None and initialize.
Apply this diff:
-def edit_app_cfg(cli, i, app_config={}):
+def edit_app_cfg(cli, i, app_config=None):
# Modify the json-rpc addresses to avoid conflict
- cluster.edit_app_cfg(
+ cluster.edit_app_cfg(
cli.home(i) / "config/app.toml",
cli.base_port(i),
- jsonmerge.merge(
+ jsonmerge.merge(
{
"json-rpc": {
"enable": True,
"address": "127.0.0.1:{EVMRPC_PORT}",
"ws-address": "127.0.0.1:{EVMRPC_PORT_WS}",
},
},
- app_config,
+ app_config or {},
),
)🧰 Tools
🪛 Ruff (0.14.1)
1034-1034: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In integration_tests/utils.py around lines 1034 to 1049, the function
edit_app_cfg uses a mutable default argument app_config={}, which can leak state
across calls; change the signature to use app_config=None and inside the
function set app_config = {} if app_config is None before calling
jsonmerge.merge, leaving the rest of the logic unchanged so each call gets a
fresh dict when no config is provided.
mmsqe
force-pushed
the
develop
branch
2 times, most recently
from
1031a0c to
3d1b97a
Compare
mmsqe
force-pushed
the
develop
branch
8 times, most recently
from
00c176e to
4ff795e
Compare
mmsqe
force-pushed
the
develop
branch
3 times, most recently
from
3530754 to
af225a5
Compare
mmsqe
force-pushed
the
develop
branch
2 times, most recently
from
8740bc5 to
93a8cd1
Compare
mmsqe
force-pushed
the
develop
branch
3 times, most recently
from
42534ad to
ea5a393
Compare
mmsqe
force-pushed
the
develop
branch
2 times, most recently
from
23fdd2f to
663911f
Compare
mmsqe
force-pushed
the
develop
branch
2 times, most recently
from
ddc0759 to
aed9bf9
Compare
mmsqe
force-pushed
the
develop
branch
2 times, most recently
from
1339069 to
c2c003f
Compare
2 participants
Closes #105
Summary by CodeRabbit
Release Notes