Includes frontier PRs polkadot-evm/frontier#1820 and polkadot-evm/frontier#1824#3677
Includes frontier PRs polkadot-evm/frontier#1820 and polkadot-evm/frontier#1824#3677
Conversation
librelois
added
B5-clientnoteworthy
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
librelois
dismissed stale reviews from manuelmauro and coderabbitai[bot]
via
87ab282
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pallets/ethereum-xcm/src/lib.rs (1)
357-369:⚠️ Potential issue | 🟠 MajorUse the runtime's AllowUnprotectedTxs configuration in XCM transaction validation.
Line 368 hardcodes
allow_unprotected_txs: false, which bypasses the runtime-configurableAllowUnprotectedTxssetting frompallet_ethereum::Config. If a runtime enables unprotected transactions, XCM validation will still reject them while the Ethereum RPC path allows them, creating an inconsistency.Add
pallet_ethereum::Configto the trait bounds and source the setting:Proposed changes
pub trait Config: frame_system::Config<RuntimeEvent: From<Event<Self>>> + pallet_evm::Config + + pallet_ethereum::Config {- allow_unprotected_txs: false, + allow_unprotected_txs: <T as pallet_ethereum::Config>::AllowUnprotectedTxs::get(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/ethereum-xcm/src/lib.rs` around lines 357 - 369, The XCM EVM validation hardcodes allow_unprotected_txs: false in the CheckEvmTransactionConfig (created by CheckEvmTransaction::<T::InvalidEvmTransactionError>::new) which ignores the runtime's AllowUnprotectedTxs setting; update the pallet trait bounds to require pallet_ethereum::Config and replace the hardcoded value by reading pallet_ethereum::Config::AllowUnprotectedTxs (or the associated constant/type from the pallet_ethereum::Config) so allow_unprotected_txs is sourced from the runtime configuration when constructing CheckEvmTransactionConfig (alongside the existing evm_config: T::config(), block_gas_limit, base_fee, chain_id, is_transactional fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pallets/ethereum-xcm/src/lib.rs`:
- Around line 357-369: The XCM EVM validation hardcodes allow_unprotected_txs:
false in the CheckEvmTransactionConfig (created by
CheckEvmTransaction::<T::InvalidEvmTransactionError>::new) which ignores the
runtime's AllowUnprotectedTxs setting; update the pallet trait bounds to require
pallet_ethereum::Config and replace the hardcoded value by reading
pallet_ethereum::Config::AllowUnprotectedTxs (or the associated constant/type
from the pallet_ethereum::Config) so allow_unprotected_txs is sourced from the
runtime configuration when constructing CheckEvmTransactionConfig (alongside the
existing evm_config: T::config(), block_gas_limit, base_fee, chain_id,
is_transactional fields).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
node/service/src/rpc.rs (1)
287-287: Redundant doubleArc::clone— preferArc::clone(&client).
Arc::clone(&client.clone())clones the Arc twice (one temporary clone immediately dropped). All otherArc::clonecalls in this block use the direct form; this one should too.♻️ Proposed fix
- Arc::clone(&client.clone()), + Arc::clone(&client),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/service/src/rpc.rs` at line 287, The call uses a redundant double clone: replace the expression Arc::clone(&client.clone()) with the direct form Arc::clone(&client) to avoid creating and dropping a temporary Arc; locate the occurrence where Arc::clone is invoked with client (the variable named client in the rpc module) and change it to Arc::clone(&client) so it matches the other clones in this block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node/service/src/rpc.rs`:
- Line 287: The call uses a redundant double clone: replace the expression
Arc::clone(&client.clone()) with the direct form Arc::clone(&client) to avoid
creating and dropping a temporary Arc; locate the occurrence where Arc::clone is
invoked with client (the variable named client in the rpc module) and change it
to Arc::clone(&client) so it matches the other clones in this block.
Coverage Report@@ Coverage Diff @@
## master elois/frontier-pin +/- ##
=====================================================
Coverage 77.10% 77.10% 0.00%
Files 389 389
+ Lines 76941 76943 +2
=====================================================
+ Hits 59321 59323 +2
Misses 17620 17620
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
runtime/moonbeam/src/lib.rs (1)
692-702: Consider usingConstBool<true>instead ofpub const AllowUnprotectedTxs: bool = truefor consistency with similar bool configs.The
parameter_types!block at line 694 definesAllowUnprotectedTxsas a constant bool that's then assigned topallet_ethereum::Config::type AllowUnprotectedTxsat line 701.ConstBool<true>is already imported (line 54) and used elsewhere in this runtime (e.g.,type AllowMultipleBlocksPerSlot = ConstBool<true>at line 901), making it a more concise and idiomatic alternative.The
pallet_ethereum_xcmpallet hardcodesallow_unprotected_txs: falseindependently—it does not reference the runtime config value—so changing this parameter will only affect the directpallet_ethereumpath, not XCM transactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/moonbeam/src/lib.rs` around lines 692 - 702, Replace the bool-style parameter declaration with the generic ConstBool version: in the parameter_types! block swap the current "pub const AllowUnprotectedTxs: bool = true" to use ConstBool<true> so the runtime-level constant matches the other bool configs; then keep "type AllowUnprotectedTxs = AllowUnprotectedTxs;" in the pallet_ethereum::Config impl (no other changes needed to pallet_ethereum::Config), ensuring symbols referenced are AllowUnprotectedTxs, parameter_types!, and pallet_ethereum::Config.node/service/src/rpc.rs (1)
299-301: Add a comment on the baretrueargument to clarify it maps toallow_unprotected_txs.The unlabeled
trueat line 300 is the newallow_unprotected_txsparameter added tofc_rpc::Eth::newby the upstream frontier change. Unlike the surrounding named-variable arguments, it reads as magic. A brief inline comment prevents ambiguity for future readers, especially since the value must stay in sync with the runtime'sAllowUnprotectedTxs = true.♻️ Proposed fix
- 10, - true, + 10, + true, // allow_unprotected_txs — must match pallet_ethereum::Config::AllowUnprotectedTxs forced_parent_hashes,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/service/src/rpc.rs` around lines 299 - 301, The bare literal `true` in the call to fc_rpc::Eth::new (the argument list containing 10, true, forced_parent_hashes) corresponds to the new allow_unprotected_txs parameter and should be annotated with an inline comment to make intent explicit; update the call site in rpc.rs (the fc_rpc::Eth::new invocation) to add a short comment like /* allow_unprotected_txs: true */ next to the literal and include a note reminding maintainers to keep this value in sync with the runtime's AllowUnprotectedTxs = true.runtime/moonriver/src/lib.rs (1)
695-706: Consider usingConstBool<true>directly instead of the parameter_types wrapper forAllowUnprotectedTxs.Other boolean config items in this runtime (e.g.,
pallet_async_backing::Config::AllowMultipleBlocksPerSlotat line 901) useConstBool<true>directly in theimplblock. This would be more concise and consistent:♻️ Optional: simplify boolean config
parameter_types! { pub const PostBlockAndTxnHashes: PostLogContent = PostLogContent::BlockAndTxnHashes; - pub const AllowUnprotectedTxs: bool = true; } impl pallet_ethereum::Config for Runtime { type StateRoot = pallet_ethereum::IntermediateStateRoot<<Runtime as frame_system::Config>::Version>; type PostLogContent = PostBlockAndTxnHashes; type ExtraDataLength = ConstU32<30>; - type AllowUnprotectedTxs = AllowUnprotectedTxs; + type AllowUnprotectedTxs = ConstBool<true>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/moonriver/src/lib.rs` around lines 695 - 706, The AllowUnprotectedTxs parameter_types wrapper is unnecessary—remove the AllowUnprotectedTxs parameter_types declaration and use ConstBool<true> directly in the pallet_ethereum::Config impl: set type AllowUnprotectedTxs = ConstBool<true>; (leave PostBlockAndTxnHashes and other parameter_types intact); this matches the pattern used by pallet_async_backing::Config::AllowMultipleBlocksPerSlot and simplifies the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node/service/src/rpc.rs`:
- Around line 299-301: The bare literal `true` in the call to fc_rpc::Eth::new
(the argument list containing 10, true, forced_parent_hashes) corresponds to the
new allow_unprotected_txs parameter and should be annotated with an inline
comment to make intent explicit; update the call site in rpc.rs (the
fc_rpc::Eth::new invocation) to add a short comment like /*
allow_unprotected_txs: true */ next to the literal and include a note reminding
maintainers to keep this value in sync with the runtime's AllowUnprotectedTxs =
true.
In `@runtime/moonbeam/src/lib.rs`:
- Around line 692-702: Replace the bool-style parameter declaration with the
generic ConstBool version: in the parameter_types! block swap the current "pub
const AllowUnprotectedTxs: bool = true" to use ConstBool<true> so the
runtime-level constant matches the other bool configs; then keep "type
AllowUnprotectedTxs = AllowUnprotectedTxs;" in the pallet_ethereum::Config impl
(no other changes needed to pallet_ethereum::Config), ensuring symbols
referenced are AllowUnprotectedTxs, parameter_types!, and
pallet_ethereum::Config.
In `@runtime/moonriver/src/lib.rs`:
- Around line 695-706: The AllowUnprotectedTxs parameter_types wrapper is
unnecessary—remove the AllowUnprotectedTxs parameter_types declaration and use
ConstBool<true> directly in the pallet_ethereum::Config impl: set type
AllowUnprotectedTxs = ConstBool<true>; (leave PostBlockAndTxnHashes and other
parameter_types intact); this matches the pattern used by
pallet_async_backing::Config::AllowMultipleBlocksPerSlot and simplifies the
code.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts (1)
92-92:⚠️ Potential issue | 🟡 MinorT01 timeout not bumped consistently with T03.
T01 (
timeout: 40_000) and T03 (timeout: 120_000) both callcreateBlocks(2, [1,2,3], ...)— identical workloads. Only T03 received the bump; T01 is more likely to flake under load.🛠️ Proposed fix
- timeout: 40_000, + timeout: 120_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts` at line 92, Test T01 uses a shorter timeout (timeout: 40_000) but runs the same heavy workload as T03 (both call createBlocks(2, [1,2,3], ...)), so update T01’s timeout to match T03 (120_000) to prevent flakiness; locate the T01 test block in test-eth-fee-history.ts and change the timeout value from 40_000 to 120_000 where the timeout is set for that test.
🧹 Nitpick comments (7)
test/suites/dev/moonbase/test-contract/test-contract-methods.ts (1)
29-31: LGTM — robust improvement over positional block lookup.Fetching the transaction first to resolve its
blockHashand then assertingtoContain(instead of an index-based equality) is strictly more correct: it handles blocks with pre-existing transactions and no longer relies on ordering assumptions.One minor note:
tx.blockHash!suppresses the TypeScriptnulltype for pending transactions. In thisdevchain contextdeployContractawaits finalization beforebeforeAllresolves, so it is safe in practice. Adding an explicit assertion before line 30 would turn a potential silent-nullfailure into a clear message if the invariant ever breaks:🛡️ Optional: guard with an explicit assertion
const tx = await context.viem().getTransaction({ hash: deployHash }); + expect(tx.blockHash, "Transaction should be mined before block lookup").not.toBeNull(); const block = await context.viem().getBlock({ blockHash: tx.blockHash! }); expect(block.transactions).toContain(deployHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-contract/test-contract-methods.ts` around lines 29 - 31, Add an explicit assertion that tx.blockHash is non-null before using it: after calling context.viem().getTransaction({ hash: deployHash }) and assigning to tx, assert that tx.blockHash is defined (e.g., throw or use expect with a clear message) so the subsequent call to context.viem().getBlock({ blockHash: tx.blockHash }) cannot silently pass a null; reference the variables tx, deployHash and the calls to context.viem().getTransaction / getBlock when locating the spot to insert this guard.test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-10.ts (1)
153-161: LGTM — fetching by pre-computed block number correctly fixes the "latest" flakiness.Pre-computing
blockNumberbeforeinjectHrmpMessageAndSealand then fetching by that specific number is the right fix for the intermittent-null issue described in#3667. BecausegetBlockNumber()is awaited fresh at the start of every loop iteration, both the V1 and V2 passes compute the correct target block independently.One optional hardening: Viem's
getBlockreturn type includesnullwhen the block isn't found. Accessing.transactionsdirectly on a potentialnullwould produce an opaqueTypeErrorrather than a meaningful test failure. A lightweight guard makes the failure message clearer if this ever trips:🛡️ Optional null guard
- const ethBlock = await context.viem().getBlock({ blockNumber }); - // Input size is valid - on the limit -, expect block to include a transaction. - // That means the pallet-ethereum-xcm decoded the provided input to a BoundedVec. - expect(ethBlock.transactions.length).to.be.eq(1); + const ethBlock = await context.viem().getBlock({ blockNumber }); + // Input size is valid - on the limit -, expect block to include a transaction. + // That means the pallet-ethereum-xcm decoded the provided input to a BoundedVec. + expect(ethBlock, `block ${blockNumber} not found`).to.not.be.null; + expect(ethBlock!.transactions.length).to.be.eq(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-10.ts` around lines 153 - 161, The test fetches ethBlock via context.viem().getBlock(...) and then accesses ethBlock.transactions without guarding against getBlock returning null; update the test around injectHrmpMessageAndSeal / the ethBlock retrieval so you assert ethBlock is not null (or throw a clear assertion) before checking ethBlock.transactions.length to produce an explicit, informative test failure instead of a TypeError when the block is missing.test/suites/dev/moonbase/test-contract/test-contract-loop-cost.ts (1)
43-43: Consider guardingresultbefore the non-null assertion for a clearer failure message.
result!.hashsuppresses the TypeScript error but will still throw a runtimeTypeError: Cannot read properties of null (reading 'hash')ifcreateBlockreturns{ result: null }(e.g., on an unexpected tx rejection). An explicitexpect(result).toBeDefined()up front gives a descriptive failure rather than a confusing NPE.The underlying change — verifying
receipt.gasUsedinstead ofblock.gasUsed— is correct and semantically more precise for per-transaction gas assertions.receipt.gasUsed(bigintfrom viem) aligns with thebigintgasliterals inTestParameters, so the.toBe()comparison is type-safe.🛡️ Proposed defensive guard
- const { result } = await context.createBlock(rawSigned); + const { result } = await context.createBlock(rawSigned); + expect(result, "block creation result should not be null").toBeDefined(); expect( await context.readContract!({Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-contract/test-contract-loop-cost.ts` at line 43, The test currently uses result!.hash which can throw a runtime NPE if context.createBlock(rawSigned) returns { result: null }; add an explicit guard before dereferencing by asserting the result is defined (e.g., expect(result).toBeDefined()) immediately after const { result } = await context.createBlock(rawSigned) and similarly for the other occurrences (lines covering the second createBlock call), then continue using result.hash; also ensure the test uses receipt.gasUsed for per-transaction gas assertions (receipt.gasUsed is a bigint matching TestParameters gas literals) rather than block.gasUsed.test/suites/dev/moonbase/test-storage-growth/test-block-storage-growth.ts (1)
35-37: LGTM — correctly eliminates reliance on the "latest" block tag.Snapshotting
currentBlock + 1nbeforecreateBlock()and then querying by that specific block number is the right fix for the flakiness described in issue#3667. The+1nBigInt arithmetic is correct, and the assumption that no extra block is produced between lines 35 and 36 holds becausefoundationMethods: "dev"disables automatic block production.One minor pre-existing inconsistency: the test title (line 18) still reads "should fill a block with 61 tx at most" while the assertion now checks for
264transactions. Since line 37 is being touched in this PR, it's a good moment to align the title.📝 Suggested title update
- title: "should fill a block with 61 tx at most", + title: "should fill a block with 264 tx at most",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-storage-growth/test-block-storage-growth.ts` around lines 35 - 37, Update the test title string in test/suites/dev/moonbase/test-storage-growth/test-block-storage-growth.ts to match the assertion change: replace the existing "should fill a block with 61 tx at most" with a title that reflects the new expectation (e.g., "should fill a block with 264 tx at most") so the it/test description and the assertion for 264 transactions are consistent; locate the test by its current title string in the file and update that literal accordingly.test/suites/dev/moonbase/test-moon/test-moon-rpc.ts (1)
92-102: LGTM — usingresult.hashdirectly is more robust here.Sourcing the transaction hash from
result.hashrather thanblock.transactions[n]is the right call given the re-org fix in Frontier#1820: the old approach could race with a block being replaced under the test, producing a stale or null hash. The non-null assertionresult!is safe since all three tests pass a real transaction tocreateBlock.One minor note: if you ever want earlier failure diagnostics, swapping
result!.hashfor a guarded assertion like:- const resp = await api.rpc.moon.isTxFinalized(result!.hash as `0x${string}`); + expect(result, "createBlock should return a transaction result").toBeDefined(); + const resp = await api.rpc.moon.isTxFinalized(result!.hash as `0x${string}`);…would produce a more descriptive error if
createBlockever fails to include the transaction, instead of a bareTypeError: Cannot read properties of null.Also applies to: 111-121, 140-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-moon/test-moon-rpc.ts` around lines 92 - 102, The test should source the transaction hash from the createBlock result instead of reading block.transactions[...] to avoid races; update the calls that use block.transactions[n] to use the hash returned by context.createBlock (the result object from createViemTransaction) — e.g., use result.hash (from the result returned by context.createBlock/createViemTransaction) and optionally add a guarded assertion that result and result.hash are present before calling api.rpc.moon.isTxFinalized to get a clearer failure message; apply the same change to the other occurrences mentioned (the blocks around where result is used at the other ranges).test/suites/dev/moonbase/test-eth-tx/test-eth-tx-access-list.ts (1)
56-59: Consider guardingresult!.hashfor better test failure diagnostics.Both T01 and T02 run 100 iterations. If any block creation silently produces a null
result(e.g., transaction not included), TypeScript's non-null assertion (!) produces a bareCannot read properties of null (reading 'hash')with no context about which iteration failed. A lightweight guard surfaces a clearer message:♻️ Suggested guard (same pattern applies to T02 at lines 119–122)
- const { result } = await context.createBlock(txWithAL); + const { result } = await context.createBlock(txWithAL); + expect(result?.hash, `Block creation returned no hash for n=${n}`).toBeDefined(); const receipt = await context .viem() - .getTransactionReceipt({ hash: result!.hash as `0x${string}` }); + .getTransactionReceipt({ hash: result!.hash as `0x${string}` });Also applies to: 119-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-eth-tx/test-eth-tx-access-list.ts` around lines 56 - 59, The test currently uses a non-null assertion on result from context.createBlock which yields an unhelpful runtime error if createBlock returns null; replace the assertion with an explicit guard: after const { result } = await context.createBlock(txWithAL); add an if (!result) throw new Error(...) that includes identifying context such as the transaction payload (txWithAL), the current iteration/index if available, and any returned metadata to make failures diagnosable; apply the same explicit-null-check pattern to the other occurrence in T02 (the receipt lookup that uses result!.hash).test/suites/dev/moonbase/test-subscription/test-subscription-logs.ts (1)
32-32:tx.blockHash!non-null assertion — optional defensive guard for clearer error messagesIn viem,
Transaction.blockHashis typed0x${string} | null(null for pending transactions). The non-null assertion here is justified because the transaction is guaranteed to be mined beforelogs[0]is populated—log events only fire for confirmed transactions. However, adding an explicit guard makes this assumption visible and ensures a meaningful error message if the timing assumption ever changes.If desired, add a defensive check:
🛡️ Optional defensive guard
+ expect(tx.blockHash, "tx should be mined before log event fires").toBeTruthy(); const block = await context.viem().getBlock({ blockHash: tx.blockHash! });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-subscription/test-subscription-logs.ts` at line 32, Replace the non-null assertion on tx.blockHash used in the call to context.viem().getBlock with an explicit defensive check: verify tx.blockHash is not null before calling getBlock (e.g., if null throw or fail the test with a clear message), so the code in the block lookup path (the getBlock call and any logic around logs[0]) fails with a meaningful error rather than using the `!` operator; update references to tx.blockHash in this test to use that guard and a descriptive error mentioning the unexpected pending transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts`:
- Around line 100-105: matchExpectations contains a dead assertion because the
filter callback in the reward-length check (the `feeResults.reward.filter((item)
=> { item.length !== reward_percentiles.length; });` call) does not return a
boolean, so `failures` is always empty; fix it by making the callback return the
comparison (e.g. change to a concise arrow that returns `item.length !==
reward_percentiles.length` or add an explicit `return`), or alternatively
replace the filter with a proper loop that collects mismatches before the
`expect(failures.length).toBe(0)` assertion; refer to the `matchExpectations`
function and the `failures`/`feeResults.reward.filter` expression to locate the
change.
---
Outside diff comments:
In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts`:
- Line 92: Test T01 uses a shorter timeout (timeout: 40_000) but runs the same
heavy workload as T03 (both call createBlocks(2, [1,2,3], ...)), so update T01’s
timeout to match T03 (120_000) to prevent flakiness; locate the T01 test block
in test-eth-fee-history.ts and change the timeout value from 40_000 to 120_000
where the timeout is set for that test.
---
Nitpick comments:
In `@test/suites/dev/moonbase/test-contract/test-contract-loop-cost.ts`:
- Line 43: The test currently uses result!.hash which can throw a runtime NPE if
context.createBlock(rawSigned) returns { result: null }; add an explicit guard
before dereferencing by asserting the result is defined (e.g.,
expect(result).toBeDefined()) immediately after const { result } = await
context.createBlock(rawSigned) and similarly for the other occurrences (lines
covering the second createBlock call), then continue using result.hash; also
ensure the test uses receipt.gasUsed for per-transaction gas assertions
(receipt.gasUsed is a bigint matching TestParameters gas literals) rather than
block.gasUsed.
In `@test/suites/dev/moonbase/test-contract/test-contract-methods.ts`:
- Around line 29-31: Add an explicit assertion that tx.blockHash is non-null
before using it: after calling context.viem().getTransaction({ hash: deployHash
}) and assigning to tx, assert that tx.blockHash is defined (e.g., throw or use
expect with a clear message) so the subsequent call to context.viem().getBlock({
blockHash: tx.blockHash }) cannot silently pass a null; reference the variables
tx, deployHash and the calls to context.viem().getTransaction / getBlock when
locating the spot to insert this guard.
In `@test/suites/dev/moonbase/test-eth-tx/test-eth-tx-access-list.ts`:
- Around line 56-59: The test currently uses a non-null assertion on result from
context.createBlock which yields an unhelpful runtime error if createBlock
returns null; replace the assertion with an explicit guard: after const { result
} = await context.createBlock(txWithAL); add an if (!result) throw new
Error(...) that includes identifying context such as the transaction payload
(txWithAL), the current iteration/index if available, and any returned metadata
to make failures diagnosable; apply the same explicit-null-check pattern to the
other occurrence in T02 (the receipt lookup that uses result!.hash).
In `@test/suites/dev/moonbase/test-moon/test-moon-rpc.ts`:
- Around line 92-102: The test should source the transaction hash from the
createBlock result instead of reading block.transactions[...] to avoid races;
update the calls that use block.transactions[n] to use the hash returned by
context.createBlock (the result object from createViemTransaction) — e.g., use
result.hash (from the result returned by
context.createBlock/createViemTransaction) and optionally add a guarded
assertion that result and result.hash are present before calling
api.rpc.moon.isTxFinalized to get a clearer failure message; apply the same
change to the other occurrences mentioned (the blocks around where result is
used at the other ranges).
In `@test/suites/dev/moonbase/test-storage-growth/test-block-storage-growth.ts`:
- Around line 35-37: Update the test title string in
test/suites/dev/moonbase/test-storage-growth/test-block-storage-growth.ts to
match the assertion change: replace the existing "should fill a block with 61 tx
at most" with a title that reflects the new expectation (e.g., "should fill a
block with 264 tx at most") so the it/test description and the assertion for 264
transactions are consistent; locate the test by its current title string in the
file and update that literal accordingly.
In `@test/suites/dev/moonbase/test-subscription/test-subscription-logs.ts`:
- Line 32: Replace the non-null assertion on tx.blockHash used in the call to
context.viem().getBlock with an explicit defensive check: verify tx.blockHash is
not null before calling getBlock (e.g., if null throw or fail the test with a
clear message), so the code in the block lookup path (the getBlock call and any
logic around logs[0]) fails with a meaningful error rather than using the `!`
operator; update references to tx.blockHash in this test to use that guard and a
descriptive error mentioning the unexpected pending transaction.
In `@test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-10.ts`:
- Around line 153-161: The test fetches ethBlock via
context.viem().getBlock(...) and then accesses ethBlock.transactions without
guarding against getBlock returning null; update the test around
injectHrmpMessageAndSeal / the ethBlock retrieval so you assert ethBlock is not
null (or throw a clear assertion) before checking ethBlock.transactions.length
to produce an explicit, informative test failure instead of a TypeError when the
block is missing.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts (1)
153-166: T03 timeout appears over-provisioned.T03 creates 2 blocks × 3 transactions = 6 total transactions — far less work than T02's 11 × 10 = 110 — yet both timeouts were set to 120 000 ms. Looks like a copy-paste from T02. Consider reverting T03 to a tighter bound (e.g. the original 40 000 ms or a modest 60 000 ms) to catch genuine hangs sooner.
⏱️ Proposed timeout adjustment for T03
- timeout: 120_000, + timeout: 60_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts` around lines 153 - 166, The test's timeout is over-provisioned: in the test function that sets block_count = 2 and creates 6 transactions (variables block_count, reward_percentiles, priority_fees and the test async function invoking createBlocks and customDevRpcRequest), reduce the timeout from 120_000 to a tighter bound (e.g., 40_000 or 60_000 ms) in the test configuration so the test fails faster on hangs while still allowing normal completion.test/suites/tracing-tests/test-trace-erc20-xcm.ts (1)
120-127: Unguardedtransactions[0]access (same astest-trace-ethereum-xcm-1.ts).
transactions[0]is accessed without confirming the array is non-empty. If the XCM transaction doesn't land as expected,transactionHashbecomesundefinedand the failure propagates to T01/T02 with a cryptic error.🛡️ Proposed guard
+ expect( + (await context.viem().getBlock({ blockNumber: successfulXcmBlockNumber })).transactions.length + ).toBeGreaterThan(0); transactionHash = (await context.viem().getBlock({ blockNumber: successfulXcmBlockNumber })) .transactions[0];Or cache the block to avoid the double fetch:
- transactionHash = (await context.viem().getBlock({ blockNumber: successfulXcmBlockNumber })) - .transactions[0]; + const successfulBlock = await context.viem().getBlock({ blockNumber: successfulXcmBlockNumber }); + expect(successfulBlock.transactions.length).toBeGreaterThan(0); + transactionHash = successfulBlock.transactions[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/tracing-tests/test-trace-erc20-xcm.ts` around lines 120 - 127, The code reads transactions[0] without checking the block's transactions array which can yield undefined; change the logic to first fetch and cache the block (const block = await context.viem().getBlock({ blockNumber: successfulXcmBlockNumber })), then verify block.transactions && block.transactions.length > 0 and throw or fail with a clear error if empty, otherwise set transactionHash = block.transactions[0]; reference the variables/functions successfulXcmBlockNumber, transactionHash, context.viem().getBlock and injectHrmpMessageAndSeal when locating the change.test/suites/tracing-tests/test-trace-ethereum-xcm-1.ts (1)
113-123: Unguardedtransactions[0]access.
processedBlock.transactions[0]is accessed without a length check. If the XCM transaction is absent from the block,undefinedis pushed intotransactionHashessilently, and the failure only surfaces later in T01 with an opaque error. The parallel filetest-trace-ethereum-xcm-3.tsusesexpect(txHashes.length).toBe(...)before indexing — this file should do the same.🛡️ Proposed guard
+ expect(processedBlock.transactions.length).toBeGreaterThan(0); transactionHashes.push(processedBlock.transactions[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/tracing-tests/test-trace-ethereum-xcm-1.ts` around lines 113 - 123, The test unguardedly indexes processedBlock.transactions[0] which can be undefined; modify the block retrieval section (around processedBlockNumber, processedBlock, and transactionHashes) to assert the block contains at least one transaction before pushing: after calling context.viem().getBlock({ blockNumber: processedBlockNumber }) check processedBlock && Array.isArray(processedBlock.transactions) && processedBlock.transactions.length > 0 (or use an expect like expect(processedBlock.transactions.length).toBeGreaterThan(0)) and only then push processedBlock.transactions[0]; keep the surrounding flow that sends the XCM via injectHrmpMessageAndSeal and uses xcmMessage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/dev/moonbase/test-eth-fee/test-eth-fee-history.ts`:
- Around line 153-166: The test's timeout is over-provisioned: in the test
function that sets block_count = 2 and creates 6 transactions (variables
block_count, reward_percentiles, priority_fees and the test async function
invoking createBlocks and customDevRpcRequest), reduce the timeout from 120_000
to a tighter bound (e.g., 40_000 or 60_000 ms) in the test configuration so the
test fails faster on hangs while still allowing normal completion.
In `@test/suites/tracing-tests/test-trace-erc20-xcm.ts`:
- Around line 120-127: The code reads transactions[0] without checking the
block's transactions array which can yield undefined; change the logic to first
fetch and cache the block (const block = await context.viem().getBlock({
blockNumber: successfulXcmBlockNumber })), then verify block.transactions &&
block.transactions.length > 0 and throw or fail with a clear error if empty,
otherwise set transactionHash = block.transactions[0]; reference the
variables/functions successfulXcmBlockNumber, transactionHash,
context.viem().getBlock and injectHrmpMessageAndSeal when locating the change.
In `@test/suites/tracing-tests/test-trace-ethereum-xcm-1.ts`:
- Around line 113-123: The test unguardedly indexes
processedBlock.transactions[0] which can be undefined; modify the block
retrieval section (around processedBlockNumber, processedBlock, and
transactionHashes) to assert the block contains at least one transaction before
pushing: after calling context.viem().getBlock({ blockNumber:
processedBlockNumber }) check processedBlock &&
Array.isArray(processedBlock.transactions) && processedBlock.transactions.length
> 0 (or use an expect like
expect(processedBlock.transactions.length).toBeGreaterThan(0)) and only then
push processedBlock.transactions[0]; keep the surrounding flow that sends the
XCM via injectHrmpMessageAndSeal and uses xcmMessage unchanged.
…ntier#1824 (#3677) * update frontier pin * Configure AllowUnprotectedTxs to false * Fix compile error * Temporary allow unprotected txs to not break tests * fix dev tests * fix tracing tests * fix coderabbit suggestion
2 participants
What does it do?
Includes frontier PRs polkadot-evm/frontier#1824 and polkadot-evm/frontier#1820
What important points should reviewers know?
The new Frontier pin also includes polkadot-evm/frontier#1815, which adds a new configuration option,
AllowUnprotectedTxs.Setting this option to
falsebreaks many tests and requires changes in Moonwall to always provide a chainId, even for legacy transactions. This work will be addressed in a future PR.For now, this PR sets
AllowUnprotectedTxstotrue.Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?