fix: apply fork changes to v0.6.0#14
Merged
Thaleszh merged 17 commits intofeat/v0.6.0from Mar 4, 2026
Merged
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WalkthroughThis PR refactors mempool broadcasting and client context management across multiple components. It removes direct clientCtx dependency from the EVMD application interface and startup flow, instead deferring client context setup via a SetClientCtx method on the mempool. The mempool now detects duplicate transactions before broadcasting and includes enhanced gas refund logic that accepts gasUsed as a parameter and optionally uses paid fee information from context. Changes include support for MMII-format Ledger device matching, EIP-712 chain ID extraction from sign documents, and new integration and system tests validating gas refunds and transaction broadcasting behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
tests/systemtests/suite/test_suite.go (1)
136-157: MirrorSetupTestdefault arg behavior here.This method is invoked without explicit
nodeStartArgsin system tests. Line [156] should applyDefaultNodeArgs()when no args are provided, so restart behavior stays consistent withSetupTest.Proposed fix
func (s *SystemTestSuite) ModifyConsensusTimeout(t *testing.T, timeout string, nodeStartArgs ...string) { t.Helper() + + if len(nodeStartArgs) == 0 { + nodeStartArgs = DefaultNodeArgs() + } // Stop the chain if running if s.ChainStarted { s.ResetChain(t) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/systemtests/suite/test_suite.go` around lines 136 - 157, ModifyConsensusTimeout currently calls s.StartChain(t, nodeStartArgs...) directly, which means when nodeStartArgs is empty it doesn't mirror SetupTest's default behavior; update the method to check if nodeStartArgs is empty and, if so, use DefaultNodeArgs() (e.g., assign args := nodeStartArgs; if len(args)==0 { args = DefaultNodeArgs() }) and then call s.StartChain(t, args...) so restart uses the same defaults as SetupTest.tests/integration/mempool/test_setup.go (1)
84-86: Consider adding cleanup to resetAllowUnsafeSyncInsertafter tests.
AllowUnsafeSyncInsertis a package-level global variable. Setting it totruehere without a correspondingTearDownTestor cleanup deferred function could leak this state to other test suites running in the same process.Suggested cleanup pattern
// Enforces deterministic mempool state for tests evmmempool.AllowUnsafeSyncInsert = true +} + +// TearDownTest resets test-specific global state. +func (s *IntegrationTestSuite) TearDownTest() { + evmmempool.AllowUnsafeSyncInsert = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/mempool/test_setup.go` around lines 84 - 86, The test sets the global evmmempool.AllowUnsafeSyncInsert = true which can leak into other tests; add cleanup to restore its original value by capturing the current value before setting it and then either defer restoring it or reset it in the test teardown (e.g., TestMain or the suite's TearDownTest). Locate the assignment to evmmempool.AllowUnsafeSyncInsert in the test setup and wrap it so you save old := evmmempool.AllowUnsafeSyncInsert, set it to true, then ensure evmmempool.AllowUnsafeSyncInsert = old is executed after tests complete.x/vm/keeper/gas.go (1)
42-46: Clarify expected behavior whengasUsedis zero.Returning early without error when
gasUsed == 0silently ignores a potentially unexpected state. In EVM execution, even the simplest transaction consumes intrinsic gas, sogasUsed == 0may indicate a bug upstream rather than a valid scenario to handle gracefully.Consider whether this should log a warning or return an error instead of silently returning.
Optional: Add logging for visibility
// Check if gas is zero if gasUsed == 0 { - // If gas is zero, we cannot refund anything, so we return early + // If gas is zero, we cannot refund anything. This is unexpected in normal + // EVM execution but can occur in edge cases. Log for visibility. + // Note: Consider returning an error if this indicates a bug. return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/vm/keeper/gas.go` around lines 42 - 46, The early return when gasUsed == 0 silently hides a likely-bug state; instead, in the function in x/vm/keeper/gas.go where gasUsed is checked, replace the silent "return nil" with explicit handling: log a warning (including gasUsed and contextual identifiers such as tx/hash or caller) and return a descriptive error indicating unexpected zero gas (or alternatively make this behavior configurable and document it). Update the check that references gasUsed and the existing return site so callers see the failure and logs provide visibility for debugging.tests/integration/x/vm/test_gas.go (1)
200-213: Error-path cases should also assert zero balance mutation.When
RefundGasis expected to error, the test should verify fee collector balances are unchanged to catch partial side effects.✅ Suggested fix
+ finalBalances := unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress) // Check the error if tc.errContains != "" { suite.Require().ErrorContains(err, tc.errContains, "RefundGas should return an error") + suite.Require().Equal(initialBalances, finalBalances, "balances must not change on refund error") } else { suite.Require().NoError(err, "RefundGas should not return an error") } // Check the balance change -if !tc.expectedRefund.Empty() { - diff := initialBalances.Sub(unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress)...) +if tc.errContains == "" && !tc.expectedRefund.Empty() { + diff := initialBalances.Sub(finalBalances...) for _, coin := range tc.expectedRefund { suite.Require().Equal(coin.Amount, diff.AmountOf(coin.Denom)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/x/vm/test_gas.go` around lines 200 - 213, When tc.errContains is non-empty (i.e. RefundGas should error), also assert that the fee collector balances did not change by comparing initialBalances to the current balances from unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress); specifically, inside the branch where you call suite.Require().ErrorContains(err, tc.errContains, ...) add a check that the returned balances equal initialBalances (or that diff is zero), and keep the existing expectedRefund assertions only in the non-error branch (or when tc.expectedRefund is non-empty and no error expected) to avoid false positives; reference the RefundGas call, tc.errContains, initialBalances, feeAddress and unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress) to locate where to add this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ethereum/eip712/encoding.go`:
- Around line 250-254: The parseChainID function currently returns a uint64 that
is later unchecked-cast to int64 in createEIP712Domain causing silent overflow;
update parseChainID to parse the string, check that the parsed value is <=
math.MaxInt64, and return an int64 (or an error if it exceeds MaxInt64) so
createEIP712Domain no longer needs an unchecked cast—specifically modify
parseChainID to use strconv.ParseUint, compare the result against math.MaxInt64,
and return the safely converted int64 or a descriptive error.
In `@mempool/mempool.go`:
- Around line 499-506: The method defaultBroadcastTxFn uses m.clientCtx without
ensuring it was initialized; add a defensive check at the start of
defaultBroadcastTxFn to verify m.clientCtx is set (non-zero / non-nil
equivalent) and return a clear error if not (e.g., "client context not set: call
SetClientCtx before broadcasting") instead of calling broadcastEVMTransactions;
reference the ExperimentalEVMMempool.receiver defaultBroadcastTxFn and the
SetClientCtx/broadcastEVMTransactions symbols so the check is placed before the
call to broadcastEVMTransactions.
- Around line 534-536: The current conditional incorrectly rejects transactions
due to a logic bug and brittle exact string comparison; update the check around
the mempool response (the block using res.Code, res.RawLog and ethTx.Hash()) to
allow code 0 or 19 and to treat "already known" as acceptable using a substring
match: replace the condition with one that returns an error only when res.Code
is non-zero and not 19 and res.RawLog does not contain "already known" (use
strings.Contains), and add the "strings" import; keep the existing error message
formatting for fmt.Errorf that references ethTx.Hash().Hex(), res.Code and
res.RawLog.
In `@server/start.go`:
- Line 249: The mempool in startStandAlone is given defaultBroadcastTxFn while
no client context is set, so BroadcastTxSync will fail; fix it by either
assigning a no-op broadcast function to the mempool (replace
defaultBroadcastTxFn with a noop that returns success/error appropriately) or
ensure you call SetClientCtx with a valid client context before the mempool can
broadcast; specifically update startStandAlone to (a) use a no-op broadcast
callback instead of defaultBroadcastTxFn when running stand-alone, or (b)
construct and pass a proper clientCtx and call mempool.SetClientCtx(clientCtx)
so defaultBroadcastTxFn can safely call clientCtx.BroadcastTxSync.
In `@tests/integration/x/vm/test_gas.go`:
- Around line 186-188: The test currently silently returns when a table entry
has tc.leftoverGas > DefaultCoreMsgGasUsage; instead of returning, fail the test
immediately so misconfigured cases are visible—replace the bare return in the
block checking "if tc.leftoverGas > DefaultCoreMsgGasUsage" with a test failure
(e.g., t.Fatalf or t.Fatalff) that prints the offending test case (use tc.name
or the test index and include tc.leftoverGas and DefaultCoreMsgGasUsage in the
message) so the bad input is obvious.
In `@tests/systemtests/mempool/test_broadcast.go`:
- Around line 56-57: The test's maxWaitTime (currently 3s) is shorter than the
test's worst-case execution path and causes flakiness; increase maxWaitTime to a
value larger than the consensus timeout and sequential polling overhead (e.g.,
10s or consensusTimeout*2) and keep checkInterval small (100ms) so polling
remains responsive; update every occurrence of the variables maxWaitTime and
checkInterval in this test (and any duplicated blocks) so all waits use the new
longer timeout to avoid "no blocks produced" races.
- Around line 28-31: The subtests capture the outer *testing.T causing
assertions/Fatal to affect the parent; change the actions slice signature and
all call sites so each action takes a *testing.T (e.g., actions
[]func(*testing.T, TestSuite) or if TestSuite is the receiver, actions []func(t
*testing.T)) and inside TestRunTxBroadcasting where you call t.Run pass the
subtest t into each action (replace uses of the outer t within action closures
to the new parameter), update functions referenced in the diff (the actions
slice definition and each invocation of those actions in TestRunTxBroadcasting
and its subtests) so assertions and FailNow/Fatal operate on the correct subtest
t.
In `@tests/systemtests/suite/test_suite.go`:
- Around line 65-69: GetCurrentBlockHeight currently ignores the error returned
by s.EthClient.Setup and immediately uses cli, which can panic or hide the real
setup failure; change the function to check the returned error from
s.EthClient.Setup (the Setup call in GetCurrentBlockHeight) before calling
cli.BlockNumber, and call require.NoError(t, err, "failed to setup eth client
for %s", nodeID) (or handle the error appropriately) so the test fails with the
correct setup error rather than panicking when dereferencing cli.
- Around line 160-195: The setValue helper currently panics if the TOML path is
missing; change setValue(doc *tomledit.Document, newVal string, xpath ...string)
to return an error instead of panicking (e.g., return fmt.Errorf("not found:
%v", xpath)) and update callers (including ModifyConsensusTimeout) to check and
propagate the error (use require.NoError in tests at the call site). Also update
editToml callers if needed to propagate the error from the inner mutation
function so editToml returns any mutation error up to the test harness.
In `@wallets/usbwallet/hub.go`:
- Around line 158-160: The current admission logic in the hub (variables
modelMatch, info.ProductID, hub.usageID, hub.endpointID, and the devices slice
append) is too permissive because it accepts any ProductID that matches only on
ProductID>>8; tighten admission by replacing prefix-only MMII matching with a
stricter policy: implement an explicit allowlist of full ProductID values per
model/interface or require a secondary protocol validation step before appending
to devices; specifically, update the matching in the hub (where modelMatch is
computed and devices is appended) to consult a per-model allowlist or call a
validation helper (e.g., ValidateDeviceProtocol or IsAllowedProduct) that checks
full ProductID plus the UsagePage/Interface and only append to devices when that
helper returns true, and ensure callers that open devices (wallet open code
paths referenced by wallet.go and ledger.go) rely on this stronger admission or
perform mandatory protocol validation before accepting the device.
---
Nitpick comments:
In `@tests/integration/mempool/test_setup.go`:
- Around line 84-86: The test sets the global evmmempool.AllowUnsafeSyncInsert =
true which can leak into other tests; add cleanup to restore its original value
by capturing the current value before setting it and then either defer restoring
it or reset it in the test teardown (e.g., TestMain or the suite's
TearDownTest). Locate the assignment to evmmempool.AllowUnsafeSyncInsert in the
test setup and wrap it so you save old := evmmempool.AllowUnsafeSyncInsert, set
it to true, then ensure evmmempool.AllowUnsafeSyncInsert = old is executed after
tests complete.
In `@tests/integration/x/vm/test_gas.go`:
- Around line 200-213: When tc.errContains is non-empty (i.e. RefundGas should
error), also assert that the fee collector balances did not change by comparing
initialBalances to the current balances from
unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress); specifically,
inside the branch where you call suite.Require().ErrorContains(err,
tc.errContains, ...) add a check that the returned balances equal
initialBalances (or that diff is zero), and keep the existing expectedRefund
assertions only in the non-error branch (or when tc.expectedRefund is non-empty
and no error expected) to avoid false positives; reference the RefundGas call,
tc.errContains, initialBalances, feeAddress and
unitNetwork.App.GetBankKeeper().GetAllBalances(ctx, feeAddress) to locate where
to add this assertion.
In `@tests/systemtests/suite/test_suite.go`:
- Around line 136-157: ModifyConsensusTimeout currently calls s.StartChain(t,
nodeStartArgs...) directly, which means when nodeStartArgs is empty it doesn't
mirror SetupTest's default behavior; update the method to check if nodeStartArgs
is empty and, if so, use DefaultNodeArgs() (e.g., assign args := nodeStartArgs;
if len(args)==0 { args = DefaultNodeArgs() }) and then call s.StartChain(t,
args...) so restart uses the same defaults as SetupTest.
In `@x/vm/keeper/gas.go`:
- Around line 42-46: The early return when gasUsed == 0 silently hides a
likely-bug state; instead, in the function in x/vm/keeper/gas.go where gasUsed
is checked, replace the silent "return nil" with explicit handling: log a
warning (including gasUsed and contextual identifiers such as tx/hash or caller)
and return a descriptive error indicating unexpected zero gas (or alternatively
make this behavior configurable and document it). Update the check that
references gasUsed and the existing return site so callers see the failure and
logs provide visibility for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ddddd59-b7db-45a7-98c2-fe423ff632b4
📒 Files selected for processing (17)
ethereum/eip712/encoding.goevmd/app.goevmd/mempool.gomempool/check_tx.gomempool/mempool.gorpc/backend/call_tx.goserver/start.gotests/integration/mempool/test_setup.gotests/integration/x/vm/test_gas.gotests/integration/x/vm/test_state_transition.gotests/systemtests/main_test.gotests/systemtests/mempool/test_broadcast.gotests/systemtests/mempool/test_exceptions.gotests/systemtests/suite/test_suite.gowallets/usbwallet/hub.gox/vm/keeper/gas.gox/vm/keeper/state_transition.go
💤 Files with no reviewable changes (3)
- tests/systemtests/mempool/test_exceptions.go
- evmd/mempool.go
- evmd/app.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Applies the following on top of v0.6.0: