Conversation
ameten
requested review from
Mo-Hussain,
kamiyaa,
paulbalaji and
yjamin
as code owners
|
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
how confident are you that these changes are required? is there any docs we can refer to that suggest these values?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7222 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 1 1
Lines 14 14
=====================================
Misses 14 14
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds per-chain transactionOverrides to Rust and TypeScript mainnet configs; adjusts transaction-status classification precedence (favoring Mempool over PendingInclusion); and inserts an informational log when Ethereum adapter skips resubmission because gas price is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as Ethereum Adapter
participant SafeSvc as SafeService
Note over Adapter,SafeSvc #DDEFEF: Resubmission decision (new log)
Adapter->>SafeSvc: query existing tx/gasPrice
SafeSvc-->>Adapter: existingGasPrice
alt newGasPrice == existingGasPrice
Adapter->>Adapter: log "resubmission skipped — gas price unchanged"
Adapter-->>Caller: TxAlreadyExists (no resubmit)
else newGasPrice != existingGasPrice
Adapter->>SafeSvc: submit/resubmit tx with higher gasPrice
SafeSvc-->>Adapter: submission result
Adapter-->>Caller: submission result
end
sequenceDiagram
participant Classifier as Status Classifier
participant ChainStatuses as StatusCountsMap
Note over Classifier,ChainStatuses #F7F7DE: Classification precedence change
ChainStatuses->>Classifier: provide counts (mempool, pendingInclusion, others)
Classifier->>Classifier: if mempool_count > 0 => return Mempool
Classifier->>Classifier: else if pending_count > 0 => return PendingInclusion
Classifier->>Classifier: else if map not empty => DroppedByChain
Classifier-->>Caller: final status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Reason: heterogeneous edits spanning config files (Rust + TypeScript), control-flow logic change in transaction classification with test updates, and adapter logging; requires cross-file verification and test review. Possibly related PRs
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: 1
🧹 Nitpick comments (1)
typescript/infra/config/environments/mainnet3/chains.ts (1)
60-66: Consider adding an explanatory comment.The configuration looks solid, mate. The 20 gwei minimum matches what's set in the Rust config, and the values are consistent across all three fields.
However, it'd be helpful to add a brief comment explaining why these overrides are necessary for Ronin, similar to what's done for
scroll(lines 28-31) andsei(lines 35-37). This'll help folks down the road understand the reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust/main/config/mainnet_config.json(2 hunks)rust/main/lander/src/adapter/chains/ethereum/adapter.rs(1 hunks)typescript/infra/config/environments/mainnet3/chains.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/adapter/chains/ethereum/adapter.rs
rust/main/config/**
📄 CodeRabbit inference engine (CLAUDE.md)
Store all chain configuration files under rust/main/config so agents can auto-discover them
Files:
rust/main/config/mainnet_config.json
⏰ 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). (57)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-config)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-2)
- GitHub Check: cli-evm-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-evm-e2e-matrix (warp-send)
- GitHub Check: cli-evm-e2e-matrix (warp-read)
- GitHub Check: cli-evm-e2e-matrix (warp-deploy)
- GitHub Check: cli-evm-e2e-matrix (warp-init)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-evm-e2e-matrix (warp-check-3)
- GitHub Check: cli-evm-e2e-matrix (relay)
- GitHub Check: cli-evm-e2e-matrix (warp-check-2)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-evm-e2e-matrix (warp-check-1)
- GitHub Check: cli-evm-e2e-matrix (core-check)
- GitHub Check: cli-evm-e2e-matrix (core-deploy)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-2)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-submitters)
- GitHub Check: cli-evm-e2e-matrix (core-read)
- GitHub Check: cli-evm-e2e-matrix (core-init)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-1)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-1)
- GitHub Check: cli-evm-e2e-matrix (core-apply)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: cli-cosmos-e2e-matrix (core-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (warp-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (warp-read)
- GitHub Check: cli-cosmos-e2e-matrix (core-read)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: env-test-matrix (mainnet3, inevm, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: cli-cosmos-e2e-matrix (core-apply)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: cli-cosmos-e2e-matrix (core-check)
- GitHub Check: env-test-matrix (mainnet3, inevm, igp)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: cli-cross-chain-e2e-matrix (warp-deploy)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: cli-cross-chain-e2e-matrix (warp-apply)
- GitHub Check: yarn-test-run
- GitHub Check: cli-install-test-run
- GitHub Check: build-and-push-to-gcr
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: agent-configs (testnet4)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: yarn-install
- GitHub Check: lander-coverage
- GitHub Check: lint-prettier
- GitHub Check: test-rs
- GitHub Check: lint-rs
🔇 Additional comments (3)
rust/main/lander/src/adapter/chains/ethereum/adapter.rs (1)
180-183: Nice addition for observability.This log statement helps track why transaction resubmissions are being skipped, which is useful for debugging gas price issues. The
info!level makes sense here since it's operational info worth seeing without cranking up to debug level. The logging pattern's consistent with what's already in the file too.Unfortunately, we can't run clippy in this sandbox environment (no Rust toolchain available here). Could you give it a quick once-over with clippy locally to make sure it's all squeaky clean? Just run:
cd rust/main/lander cargo clippy --all-targets --all-features -- -D warningsShould only take a moment, and it's per the coding guidelines for Rust files anyway.
rust/main/config/mainnet_config.json (2)
6010-6015: Ink fee floor lined up nicely.This keeps the overrides tidy and guards against feather-light bids slipping through. All looks good from my muddy boots.
6936-6941: Ronin minimum gas tweak looks solid.The new floor brings the config in line with the updated policy and stays consistent with how the rest of the swamp is managed.
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
discussed offline, lgtm
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/lander/src/transaction/types.rs (2)
88-92: Preserve the actual drop reason instead of defaulting to DroppedByChain.When only Dropped statuses are observed, keep the real reason you saw. Makes logs and handling less muddy.
Apply this diff:
- } else if !status_counts.is_empty() { - // if the hashmap is not empty, it must mean that the hashes were dropped, - // because the hashmap is populated only if the status query was successful - return TransactionStatus::Dropped(DropReason::DroppedByChain); - } + } else if !status_counts.is_empty() { + // Only dropped statuses observed; preserve the concrete reason if present. + if let Some(reason) = status_counts + .keys() + .find_map(|k| if let TransactionStatus::Dropped(r) = k { Some(r.clone()) } else { None }) + { + return TransactionStatus::Dropped(reason); + } + return TransactionStatus::Dropped(DropReason::DroppedByChain); + }
210-210: Test update matches new precedence—nice and tidy.This asserts Mempool wins when both Mempool and PendingInclusion are present. Consider (optional) a combo case where Included and Mempool coexist to ensure Included still wins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/transaction/types.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/transaction/types.rs
⏰ 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). (4)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
🔇 Additional comments (2)
rust/main/lander/src/transaction/types.rs (2)
1-2: Re‑enable clippy and keep it strict.Per guidelines, run clippy and aim for -D warnings to catch any sneaky bits before merge.
As per coding guidelines
86-87: All downstream behavior checks out—precedence shift is safe and tested.The reordering looks solid. Ripgrep shows that pretty much every place downstream that cares about Mempool or PendingInclusion treats 'em as a pair in pattern matches (
TransactionStatus::PendingInclusion | TransactionStatus::Mempool). This happens in the inclusion and finality stages, as well as the transaction loader. Since they're grouped together, the escalation/resubmission cadence stays the same—both statuses still trigger the same path through resubmission logic.The test at lines 206–216 also confirms the new precedence explicitly: when you've got Mempool, PendingInclusion, and Dropped statuses mixed together, Mempool wins. That's what you want.
One small note per the Rust coding guidelines for this file: run cargo clippy to catch any linting issues before merging.
2 participants
Description
change min gas fees for Ronin and Ink
Backward compatibility
Yes
Testing
Manual
Summary by CodeRabbit
New Features
Bug Fixes
Chores