Open
Conversation
manuelmauro
added
B0-silent
📝 WalkthroughWalkthroughAdds a pending-deposit mechanism: XCM deposits to assets frozen with FrozenXcmDepositAllowed are recorded in Changes
Sequence DiagramsequenceDiagram
participant XCM as XCM Sender
participant Pallet as ForeignAssets Pallet
participant Storage as PendingDeposits
participant Assets as Assets Storage
rect rgba(200, 150, 255, 0.5)
note over XCM,Pallet: XCM deposit while asset is FrozenXcmDepositAllowed
XCM->>Pallet: deposit_asset(location, amount, beneficiary)
Pallet->>Assets: match_asset(location) -> (asset_id, owner, amount, status)
Pallet->>Storage: PendingDeposits[asset_id, beneficiary] += amount
Pallet->>Pallet: Emit PendingDepositRecorded
end
rect rgba(150, 200, 255, 0.5)
note over XCM,Assets: Asset status changed to Active (unfreeze)
XCM->>Assets: unfreeze_asset(asset_id)
end
rect rgba(150, 255, 150, 0.5)
note over Pallet,Storage: Claim pending deposit
XCM->>Pallet: claim_pending_deposit(asset_id, beneficiary)
Pallet->>Assets: verify status == Active
Pallet->>Storage: read & remove PendingDeposits[asset_id, beneficiary]
Pallet->>Assets: mint amount to beneficiary
Pallet->>Pallet: Emit PendingDepositClaimed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Contributor
Coverage Report@@ Coverage Diff @@
## master manuel/allow-xcm-deposits-on-freeze +/- ##
=======================================================================
+ Coverage 77.10% 77.16% +0.06%
Files 389 389
+ Lines 76941 77238 +297
=======================================================================
+ Hits 59321 59594 +273
+ Misses 17620 17644 +24
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pallets/moonbeam-foreign-assets/src/tests.rs (1)
426-481: Consider asserting beneficiary balance and post-deposit pause state.The test verifies the deposit call succeeds, but doesn't assert that:
- The beneficiary actually received the minted tokens (e.g.,
balance_of== 200 after two deposits of 100).- The ERC-20 contract is still paused after the temporary unpause/mint/pause sequence, confirming the re-pause worked.
Adding these assertions would make the test more robust against a scenario where
deposit_assetreturnsOk(())but the unpause/mint/pause sequence silently breaks (e.g., re-pause fails but the error is swallowed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/tests.rs` around lines 426 - 481, Add assertions that the beneficiary's ERC-20 balance increased and that the ERC-20 contract remains paused after the deposit/unpause/mint/pause sequence: after the two successful calls to <EvmForeignAssets as xcm_executor::traits::TransactAsset>::deposit_asset use the beneficiary address derived from beneficiary_location (the AccountKey20 [1u8;20]) to call the deployed ERC‑20 contract's balance_of and assert it equals 200 (100 per deposit), and call the ERC‑20 contract's paused (or is_paused) view and assert it returns true; keep references to EvmForeignAssets::deposit_asset, EvmForeignAssets::freeze_foreign_asset, and EvmForeignAssets::assets_by_location to locate relevant setup.pallets/moonbeam-foreign-assets/src/lib.rs (1)
759-777: Solid atomicity design — unpause/mint/pause inside a single storage transaction.The
with_storage_layerwrapper correctly ensures that if the mint or re-pause fails, the unpause is rolled back, preventing the contract from being left in an unpaused state. Theneeds_unpauseflag cleanly skips the unpause/pause calls for active assets.One minor observation: the
.map_err(|_| ...)on lines 766 and 773 discards the originalError<T>fromerc20_unpause/erc20_pause. This is fine for XCM context (callers only see an opaque XCM error), but if debugging becomes difficult, consider logging the original error before discarding it.💡 Optional: log before discarding the error
if needs_unpause { EvmCaller::<T>::erc20_unpause(asset_id) - .map_err(|_| evm::EvmError::EvmCallFail("unpause fail".into()))?; + .map_err(|e| { + log::debug!("erc20_unpause failed during XCM deposit: {:?}", e); + evm::EvmError::EvmCallFail("unpause fail".into()) + })?; }if needs_unpause { EvmCaller::<T>::erc20_pause(asset_id) - .map_err(|_| evm::EvmError::EvmCallFail("pause fail".into()))?; + .map_err(|e| { + log::debug!("erc20_pause failed during XCM deposit: {:?}", e); + evm::EvmError::EvmCallFail("pause fail".into()) + })?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 759 - 777, The error mapping for EvmCaller::<T>::erc20_unpause and EvmCaller::<T>::erc20_pause is currently discarding the original Error<T> via .map_err(|_| ...); update these call sites so the original error is preserved or logged before converting to evm::EvmError (e.g., capture the err in the closure and call a logger or include its Display in the evm::EvmError::EvmCallFail message) within the with_storage_layer transaction around erc20_unpause/erc20_mint_into/erc20_pause so debugging retains the underlying error details while keeping the same external error type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pallets/moonbeam-foreign-assets/src/lib.rs`:
- Around line 759-777: The error mapping for EvmCaller::<T>::erc20_unpause and
EvmCaller::<T>::erc20_pause is currently discarding the original Error<T> via
.map_err(|_| ...); update these call sites so the original error is preserved or
logged before converting to evm::EvmError (e.g., capture the err in the closure
and call a logger or include its Display in the evm::EvmError::EvmCallFail
message) within the with_storage_layer transaction around
erc20_unpause/erc20_mint_into/erc20_pause so debugging retains the underlying
error details while keeping the same external error type.
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 426-481: Add assertions that the beneficiary's ERC-20 balance
increased and that the ERC-20 contract remains paused after the
deposit/unpause/mint/pause sequence: after the two successful calls to
<EvmForeignAssets as xcm_executor::traits::TransactAsset>::deposit_asset use the
beneficiary address derived from beneficiary_location (the AccountKey20
[1u8;20]) to call the deployed ERC‑20 contract's balance_of and assert it equals
200 (100 per deposit), and call the ERC‑20 contract's paused (or is_paused) view
and assert it returns true; keep references to EvmForeignAssets::deposit_asset,
EvmForeignAssets::freeze_foreign_asset, and EvmForeignAssets::assets_by_location
to locate relevant setup.
librelois
reviewed
Feb 18, 2026
librelois
requested changes
Feb 18, 2026
Collaborator
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
This will produce 3 eth-xcm transactions under the hood, it's better to create a new EvmCaller method erc20_mint_into_paused that create an evm call that call the batch precompile under the hood. You will have to write the code that encode the batch call properly
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pallets/moonbeam-foreign-assets/src/evm.rs (1)
93-96:⚠️ Potential issue | 🟡 MinorStray
+in theMintIntoFailerror string is a debug artifact that now surfaces on the new path.Line 95:
"Erc20 contract call mintInto fail+"— the trailing+is misleading in production logs and is now reachable via botherc20_mint_intoand the newerc20_mint_into_pausedpath.🐛 Proposed fix
- XcmError::FailedToTransactAsset("Erc20 contract call mintInto fail+") + XcmError::FailedToTransactAsset("Erc20 contract call mintInto fail")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/evm.rs` around lines 93 - 96, The EvmError::MintIntoFail match arm returns an XcmError::FailedToTransactAsset with a message containing a stray '+'; update the message in the MintIntoFail branch to remove the trailing plus so it reads e.g. "Erc20 contract call mintInto fail" (keep the log::debug line unchanged), ensuring the XcmError::FailedToTransactAsset invocation inside the EvmError::MintIntoFail arm is corrected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallets/moonbeam-foreign-assets/src/evm.rs`:
- Around line 41-52: Add a measured "highest failure: X" comment to the
ERC20_MINT_INTO_PAUSED_GAS_LIMIT constant so it matches the other ERC20_* gas
constants (replace the current descriptive breakdown with a single measured
max-failure value), and extract the BATCH_PRECOMPILE_ADDRESS raw H160 bytes into
a single shared public constant (e.g., pub const BATCH_PRECOMPILE_ADDRESS: H160)
in a common crate/module used by both evm.rs and the RPC/service code; update
references in evm.rs (and other places using the hex literal) to import and use
that shared constant to prevent duplication.
- Around line 225-307: The XCM DepositAsset weighing ignores the expensive
paused-asset path: when FrozenXcmDepositAllowed assets call
erc20_mint_into_paused() they use ~600k gas vs ~160k for normal deposits, but
deposit_asset() still uses the hardcoded DepositAsset weight; update
deposit_asset() to account for the paused-mint path by using the existing
weight_of_erc20_mint() helper or by branching on the asset's
FrozenXcmDepositAllowed status and applying a worst-case weight (or the
dynamically computed weight) so XCM fees and message weight reflect the 3.75×
cost; touch functions/methods deposit_asset, erc20_mint_into_paused,
weight_of_erc20_mint and the FrozenXcmDepositAllowed check to implement the
conditional weight selection and ensure XCM fee calculation uses the adjusted
weight.
---
Outside diff comments:
In `@pallets/moonbeam-foreign-assets/src/evm.rs`:
- Around line 93-96: The EvmError::MintIntoFail match arm returns an
XcmError::FailedToTransactAsset with a message containing a stray '+'; update
the message in the MintIntoFail branch to remove the trailing plus so it reads
e.g. "Erc20 contract call mintInto fail" (keep the log::debug line unchanged),
ensuring the XcmError::FailedToTransactAsset invocation inside the
EvmError::MintIntoFail arm is corrected.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallets/moonbeam-foreign-assets/src/evm.rs`:
- Around line 247-253: The computed selector for batchAll is wrong: update the
computation so it matches the selector used in Batch.sol (0x96e292b8) — either
by changing the keccak256! input string to the exact Solidity function signature
Batch.sol uses for batchAll or by hardcoding the selector constant to
0x96e292b8; modify the symbols batch_all_hash and batch_all_selector (and the
keccak256! invocation) so the resulting u32 equals 0x96e292b8, ensuring
precompile dispatch for batchAll(address[],uint256[],bytes[],uint64[]) matches
the Solidity contract.
---
Duplicate comments:
In `@pallets/moonbeam-foreign-assets/src/evm.rs`:
- Around line 49-53: BATCH_PRECOMPILE_ADDRESS is hard-coded here as raw H160
bytes and likely duplicated elsewhere; extract this value into a single shared
constant (e.g., a pub const BATCH_PRECOMPILE_ADDRESS: H160 in a common
crate/module) and replace local definitions in evm.rs and other copies with an
import/use of that shared symbol so all locations reference the single canonical
value and avoid silent drift.
- Line 42: The comment for the constant ERC20_MINT_INTO_PAUSED_GAS_LIMIT does
not follow the project convention of annotating the measured worst-case failure
gas (e.g., "// highest failure: 154_000"); run a worst-case gas measurement for
the sequence represented by ERC20_MINT_INTO_PAUSED_GAS_LIMIT (unpause + mintInto
+ pause via batch) and update the trailing comment to the canonical form "//
highest failure: <measured_value>" using the measured numeric value so it
matches the other ERC20_*_GAS_LIMIT constants.
- Around line 226-308: The deposit_asset flow currently uses a hardcoded weight
that ignores the paused-mint (erc20_mint_into_paused) path which consumes ~3.75×
more gas; update deposit_asset to branch on the FrozenXcmDepositAllowed status
(or otherwise detect when erc20_mint_into_paused will be invoked) and use
T::GasWeightMapping::gas_to_weight with ERC20_MINT_INTO_PAUSED_GAS_LIMIT for
that path (and the existing lower gas limit for the normal path), or add a
separate weight constant for the paused-mint branch and apply it when calling
erc20_mint_into_paused so XCM DepositAsset weights reflect the real gas usage.
Ensure the branch references the FrozenXcmDepositAllowed check and the
ERC20_MINT_INTO_PAUSED_GAS_LIMIT/erc20_mint_into_paused symbols so the correct
weight is selected.
Collaborator
|
@manuelmauro I have an idea for a better solution: create a new double storage map to store the received tokens when the asset is paused, and add a new extrinsic to claim these tokens once the asset is unpaused. This way, we don’t need extra EVM calls during XCM execution or any changes to the ERC20 contract bytecode. |
Contributor
Author
Nice idea! Addressed in 4a869aa |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pallets/moonbeam-foreign-assets/src/lib.rs (2)
345-350:PendingDepositsstorage is unbounded — consider documenting the growth model.The
StorageDoubleMaphas no bounds on the number of entries. While each(asset_id, beneficiary)pair stores a single accumulatingU256, the number of unique beneficiaries receiving XCM deposits while an asset is frozen could grow over time. Entries persist until individually claimed viaclaim_pending_deposit.This is inherent to the design, but worth documenting explicitly (e.g., in a doc comment on the storage item) so future maintainers understand the expected growth pattern and can plan governance-level mitigations if needed (e.g., batch claim tooling).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 345 - 350, Add a doc comment to the PendingDeposits storage declaration explaining that StorageDoubleMap<..., AssetId, ..., H160, U256, ...> is unbounded in the number of (asset_id, beneficiary) entries, that values accumulate via checked_add and persist until claim_pending_deposit is called, and describe the expected growth model and operational mitigations (e.g., governance-level batch claim tooling or periodic cleanup) so future maintainers understand the persistence and scaling implications.
586-626:claim_pending_deposit— add a comment documenting the transactional guarantee for thetake+ mint sequence.
PendingDeposits::take(line 609) removes the deposit from storage before the EVM mint (line 614–617). If the mint fails, the operation is safe only because#[pallet::call]wraps the entire dispatchable inframe_support::storage::with_storage_layer, rolling back all storage mutations on error. This transactional guarantee is implicit and relies on FRAME's default behavior; adding a brief comment near thetakewould clarify this invariant for future maintainers and prevent accidental breakage if the code is refactored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 586 - 626, The code removes the pending deposit with PendingDeposits::take before calling EvmCaller::erc20_mint_into, relying on frame_support::storage::with_storage_layer to roll back storage on EVM call failure; add a concise comment in claim_pending_deposit next to the PendingDeposits::take line documenting that the take happens prior to the mint and that the surrounding frame_support::storage::with_storage_layer provides the transactional rollback guarantee so future changes won't accidentally break this invariant.pallets/moonbeam-foreign-assets/src/tests.rs (1)
494-558: Consider adding an overflow test for pending deposit accumulation.The
deposit_assetcode useschecked_addand returnsXcmError::Overflowon U256 overflow, but there's no test exercising this path. While U256 overflow is practically unlikely, testing the error branch improves confidence in the checked arithmetic handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/tests.rs` around lines 494 - 558, Add a unit test that exercises the U256 overflow path in EvmForeignAssets::deposit_asset: create the foreign asset and freeze it (as in pending_deposits_accumulate), set the pending_deposits for a beneficiary to U256::MAX (or perform a deposit of U256::MAX) and then call deposit_asset with any positive amount (e.g. 1) and assert it returns Err(xcm::latest::XcmError::Overflow) and that pending_deposits is unchanged; name the test e.g. pending_deposits_overflow and reuse symbols from the existing test (EvmForeignAssets::create_foreign_asset, EvmForeignAssets::freeze_foreign_asset, <EvmForeignAssets as xcm_executor::traits::TransactAsset>::deposit_asset, EvmForeignAssets::pending_deposits).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pallets/moonbeam-foreign-assets/src/lib.rs`:
- Around line 345-350: Add a doc comment to the PendingDeposits storage
declaration explaining that StorageDoubleMap<..., AssetId, ..., H160, U256, ...>
is unbounded in the number of (asset_id, beneficiary) entries, that values
accumulate via checked_add and persist until claim_pending_deposit is called,
and describe the expected growth model and operational mitigations (e.g.,
governance-level batch claim tooling or periodic cleanup) so future maintainers
understand the persistence and scaling implications.
- Around line 586-626: The code removes the pending deposit with
PendingDeposits::take before calling EvmCaller::erc20_mint_into, relying on
frame_support::storage::with_storage_layer to roll back storage on EVM call
failure; add a concise comment in claim_pending_deposit next to the
PendingDeposits::take line documenting that the take happens prior to the mint
and that the surrounding frame_support::storage::with_storage_layer provides the
transactional rollback guarantee so future changes won't accidentally break this
invariant.
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 494-558: Add a unit test that exercises the U256 overflow path in
EvmForeignAssets::deposit_asset: create the foreign asset and freeze it (as in
pending_deposits_accumulate), set the pending_deposits for a beneficiary to
U256::MAX (or perform a deposit of U256::MAX) and then call deposit_asset with
any positive amount (e.g. 1) and assert it returns
Err(xcm::latest::XcmError::Overflow) and that pending_deposits is unchanged;
name the test e.g. pending_deposits_overflow and reuse symbols from the existing
test (EvmForeignAssets::create_foreign_asset,
EvmForeignAssets::freeze_foreign_asset, <EvmForeignAssets as
xcm_executor::traits::TransactAsset>::deposit_asset,
EvmForeignAssets::pending_deposits).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pallets/moonbeam-foreign-assets/src/lib.rs (2)
825-838: Pending deposit accumulation path is not wrapped in a storage transaction.This is safe as-is because nothing can fail after the
PendingDeposits::inserton line 831 (deposit_eventis infallible). However, it's inconsistent with the active-mint path (line 842) and the claim path (line 616), both of which usewith_storage_layer. Wrapping this block too would make the code uniformly defensive against future additions to this branch.♻️ Suggested defensive wrapping
if matches!(asset_status, AssetStatus::FrozenXcmDepositAllowed) { - let total_pending = PendingDeposits::<T>::get(asset_id, beneficiary) - .unwrap_or(U256::zero()) - .checked_add(amount) - .ok_or(XcmError::Overflow)?; - - PendingDeposits::<T>::insert(asset_id, beneficiary, total_pending); - - Pallet::<T>::deposit_event(Event::PendingDepositRecorded { - asset_id, - beneficiary, - amount, - total_pending, - }); + frame_support::storage::with_storage_layer(|| { + let total_pending = PendingDeposits::<T>::get(asset_id, beneficiary) + .unwrap_or(U256::zero()) + .checked_add(amount) + .ok_or(XcmError::Overflow)?; + + PendingDeposits::<T>::insert(asset_id, beneficiary, total_pending); + + Pallet::<T>::deposit_event(Event::PendingDepositRecorded { + asset_id, + beneficiary, + amount, + total_pending, + }); + + Ok(()) + })?; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 825 - 838, The PendingDeposit accumulation branch for AssetStatus::FrozenXcmDepositAllowed should be wrapped in the same storage transaction helper used elsewhere to be defensive against future fallible ops: enclose the logic that calls PendingDeposits::<T>::get, computes total_pending, PendingDeposits::<T>::insert and Pallet::<T>::deposit_event inside with_storage_layer(...) (the same helper used by the active-mint and claim paths) so the read-modify-write and event emission occur within a storage-layer transaction; locate the branch matching AssetStatus::FrozenXcmDepositAllowed and wrap the get/checked_add/insert/event sequence with with_storage_layer to match existing patterns.
596-606: Consider adding a debug log before discarding the original error on the EVM mint failure path.On line 620, the original error from
with_storage_layeris discarded viamap_err(|_| ...). During incident triage, the underlying EVM revert reason can be valuable. Alog::debug!orlog::warn!before mapping would preserve this information without changing the error semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 596 - 606, The EVM mint failure path currently discards the underlying error via map_err(|_| ...) in the call to with_storage_layer; update the branch where with_storage_layer(...).map_err(|_| ...) is used to first log the original error (e.g. log::debug! or log::warn! with the captured `err`) before mapping it to the existing pallet error, so change map_err(|_| ...) to map_err(|err| { log::debug!(target: "pallet::moonbeam_foreign_assets", "EVM mint failed: {:?}", err); /* then return the same mapped error */ Error::<T>::... }) and add the log crate import if missing; keep the mapped error semantics unchanged and reference the with_storage_layer call and the map_err closure in your edit.pallets/moonbeam-foreign-assets/src/tests.rs (1)
426-824: Consider adding event assertions forPendingDepositRecordedandPendingDepositClaimed.The existing test suite (e.g.,
create_foreign_and_freeze_unfreeze_using_xcm) usesexpect_eventsto verify emitted events. The new tests verify storage state but don't assert that the expected events were emitted. Adding event checks for at least the happy paths (xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_assetandclaim_pending_deposit_success) would increase confidence in the event-driven off-chain integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/tests.rs` around lines 426 - 824, Add event assertions: in the test xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset, after the deposit that writes to PendingDeposits assert the PendingDepositRecorded event was emitted (use expect_events or match on System::events() for PendingDepositRecorded with asset id 1, beneficiary H160 and amount U256::from(100)); in claim_pending_deposit_success, after calling EvmForeignAssets::claim_pending_deposit assert PendingDepositClaimed was emitted (with asset id 1, beneficiary H160 and amount U256::from(500)) and then assert the PendingDeposits storage is cleared; reference the test names xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset and claim_pending_deposit_success and the event identifiers PendingDepositRecorded and PendingDepositClaimed when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 813-822: The test currently only checks result.is_err() for the
call to EvmForeignAssets::deposit_asset; instead assert the exact error variant
to ensure the FrozenXcmDepositForbidden path yields the expected
MatchError::AssetNotHandled (which maps to xcm::latest::Error::AssetNotFound).
Update the assertion to compare result.unwrap_err() (or pattern-match result)
against xcm::latest::Error::AssetNotFound.into() (or
MatchError::AssetNotHandled.into()) so the test fails if a different error is
returned from deposit_asset.
---
Nitpick comments:
In `@pallets/moonbeam-foreign-assets/src/lib.rs`:
- Around line 825-838: The PendingDeposit accumulation branch for
AssetStatus::FrozenXcmDepositAllowed should be wrapped in the same storage
transaction helper used elsewhere to be defensive against future fallible ops:
enclose the logic that calls PendingDeposits::<T>::get, computes total_pending,
PendingDeposits::<T>::insert and Pallet::<T>::deposit_event inside
with_storage_layer(...) (the same helper used by the active-mint and claim
paths) so the read-modify-write and event emission occur within a storage-layer
transaction; locate the branch matching AssetStatus::FrozenXcmDepositAllowed and
wrap the get/checked_add/insert/event sequence with with_storage_layer to match
existing patterns.
- Around line 596-606: The EVM mint failure path currently discards the
underlying error via map_err(|_| ...) in the call to with_storage_layer; update
the branch where with_storage_layer(...).map_err(|_| ...) is used to first log
the original error (e.g. log::debug! or log::warn! with the captured `err`)
before mapping it to the existing pallet error, so change map_err(|_| ...) to
map_err(|err| { log::debug!(target: "pallet::moonbeam_foreign_assets", "EVM mint
failed: {:?}", err); /* then return the same mapped error */ Error::<T>::... })
and add the log crate import if missing; keep the mapped error semantics
unchanged and reference the with_storage_layer call and the map_err closure in
your edit.
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 426-824: Add event assertions: in the test
xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset, after the deposit that
writes to PendingDeposits assert the PendingDepositRecorded event was emitted
(use expect_events or match on System::events() for PendingDepositRecorded with
asset id 1, beneficiary H160 and amount U256::from(100)); in
claim_pending_deposit_success, after calling
EvmForeignAssets::claim_pending_deposit assert PendingDepositClaimed was emitted
(with asset id 1, beneficiary H160 and amount U256::from(500)) and then assert
the PendingDeposits storage is cleared; reference the test names
xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset and
claim_pending_deposit_success and the event identifiers PendingDepositRecorded
and PendingDepositClaimed when adding these checks.
librelois
requested changes
Feb 20, 2026
Collaborator
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
Please add a new end2end dev test with a scenario covering: freeze -> deposit -> unfreeze -> permissionless claim
Contributor
Author
Addressed in 1461c00 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/suites/dev/moonbase/test-assets/test-foreign-assets-xcm-pending-deposits.ts (1)
127-130: Prefer a typed codec accessor overBigInt(…as string)to avoid silent breakage on encoding changes.
pendingDepositis a SCALE-decoded U256; calling.toJSON()then casting tostringand wrapping inBigInt()works today (polkadot-js returns a"0x…"hex string for large values) but is fragile. A typed query or using the codec's.toBigInt()/.toString()directly is more robust.♻️ Proposed fix
- const pendingDeposit = ( - await context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId, beneficiary) - ).toJSON(); - expect(BigInt(pendingDeposit as string)).to.eq(depositAmount); + const pendingDeposit = ( + await context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId, beneficiary) + ); + expect(pendingDeposit.toString() !== "0" && BigInt(pendingDeposit.toHex())).to.eq(depositAmount);Or, if the codec exposes
.toBigInt():- const pendingDeposit = ( - await context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId, beneficiary) - ).toJSON(); - expect(BigInt(pendingDeposit as string)).to.eq(depositAmount); + const pendingDeposit = ( + await context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId, beneficiary) + ); + expect(pendingDeposit.toBigInt()).to.eq(depositAmount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-assets/test-foreign-assets-xcm-pending-deposits.ts` around lines 127 - 130, The test currently calls .toJSON() on the SCALE-decoded U256 and coerces it via BigInt(pendingDeposit as string), which is fragile; instead obtain a typed codec or use the codec's native accessor on the returned value from context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId, beneficiary) (e.g. call .toBigInt() or .toString() on the codec instance) and compare that result to depositAmount; update the pendingDeposit assignment and the expect assertion to use the codec's .toBigInt() (or a strongly typed query result) rather than BigInt(... as string) to avoid encoding breakage.pallets/moonbeam-foreign-assets/src/lib.rs (1)
817-848: Add an exhaustivematchafter theFrozenXcmDepositForbiddenearly-return to guard against futureAssetStatusvariants.The
elsebranch currently silently routes any unknown future variant to the Active (EVM-mint) path. If a newAssetStatusvariant is introduced, the compiler will not flag this. Preserving the early return forFrozenXcmDepositForbidden(before beneficiary resolution) maintains the current error-priority semantics.♻️ Proposed refactor
if let AssetStatus::FrozenXcmDepositForbidden = asset_status { return Err(XcmError::FailedToTransactAsset( "asset is frozen and XCM deposits are forbidden", )); } let beneficiary = T::XcmLocationToH160::convert_location(who) .ok_or(MatchError::AccountIdConversionFailed)?; - if matches!(asset_status, AssetStatus::FrozenXcmDepositAllowed) { - let total_pending = PendingDeposits::<T>::get(asset_id, beneficiary) - .unwrap_or(U256::zero()) - .checked_add(amount) - .ok_or(XcmError::Overflow)?; - - PendingDeposits::<T>::insert(asset_id, beneficiary, total_pending); - - Pallet::<T>::deposit_event(Event::PendingDepositRecorded { - asset_id, - beneficiary, - amount, - total_pending, - }); - } else { - // We perform the evm transfers in a storage transaction to ensure - // that if it fails any contract storage changes are rolled back. - frame_support::storage::with_storage_layer(|| { - EvmCaller::<T>::erc20_mint_into(contract_address, beneficiary, amount) - })?; - } + match asset_status { + AssetStatus::FrozenXcmDepositAllowed => { + let total_pending = PendingDeposits::<T>::get(asset_id, beneficiary) + .unwrap_or(U256::zero()) + .checked_add(amount) + .ok_or(XcmError::Overflow)?; + PendingDeposits::<T>::insert(asset_id, beneficiary, total_pending); + Pallet::<T>::deposit_event(Event::PendingDepositRecorded { + asset_id, + beneficiary, + amount, + total_pending, + }); + } + AssetStatus::Active => { + // We perform the evm transfers in a storage transaction to ensure + // that if it fails any contract storage changes are rolled back. + frame_support::storage::with_storage_layer(|| { + EvmCaller::<T>::erc20_mint_into(contract_address, beneficiary, amount) + })?; + } + AssetStatus::FrozenXcmDepositForbidden => { + unreachable!("FrozenXcmDepositForbidden handled above") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/lib.rs` around lines 817 - 848, Replace the current if/matches check on asset_status with an exhaustive match over AssetStatus (after the existing early-return for AssetStatus::FrozenXcmDepositForbidden and after resolving beneficiary) so each known variant is handled explicitly: handle AssetStatus::FrozenXcmDepositAllowed by computing total_pending, inserting PendingDeposits::<T>::insert(...) and emitting Event::PendingDepositRecorded, handle the Active/expected minting variant by performing the storage-layer EvmCaller::<T>::erc20_mint_into(contract_address, beneficiary, amount) call, and add a catch-all arm that returns a clear XcmError (or MatchError) for any unknown future variant; reference match_asset, asset_status, PendingDeposits, beneficiary and EvmCaller::erc20_mint_into when making these changes so the compiler will force exhaustiveness on AssetStatus.pallets/moonbeam-foreign-assets/src/tests.rs (1)
612-688: Consider assertingPendingDepositRecorded/PendingDepositClaimedevents in the new deposit and claim tests.
claim_pending_deposit_successverifies storage state and balance but never checks thatPendingDepositClaimedwas emitted. Similarly,xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_assetandpending_deposits_accumulatedon't assertPendingDepositRecorded. Adding event assertions prevents silent regressions where storage is updated correctly but events are dropped or malformed.♻️ Example: add event assertion to `claim_pending_deposit_success` (lines 676-686)
// Pending deposit should be cleared assert_eq!( EvmForeignAssets::pending_deposits(1, beneficiary_h160), None ); + // Verify PendingDepositClaimed event was emitted + expect_events(vec![ + Event::PendingDepositClaimed { + asset_id: 1, + beneficiary: beneficiary_h160, + amount: U256::from(500), + } + ]); + // Verify beneficiary received the tokensAlso applies to: 426-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallets/moonbeam-foreign-assets/src/tests.rs` around lines 612 - 688, The test claim_pending_deposit_success should also assert the emitted events to catch regressions: after the deposit call (TransactAsset::deposit_asset) assert that PendingDepositRecorded (from the EvmForeignAssets pallet events) was emitted with the expected asset id, beneficiary and amount, and after claim_pending_deposit assert that PendingDepositClaimed was emitted with the expected asset id, beneficiary and amount; add similar PendingDepositRecorded assertions to xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset and pending_deposits_accumulate tests to ensure events are emitted alongside the storage and balance checks (look for EvmForeignAssets::pending_deposits, claim_pending_deposit, and the pallet event variants PendingDepositRecorded/PendingDepositClaimed to locate the right spots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 813-824: Test assertion is correct and should remain as-is: ensure
the call to <EvmForeignAssets as
xcm_executor::traits::TransactAsset>::deposit_asset(...) continues to be
asserted against Err(XcmError::FailedToTransactAsset("asset is frozen and XCM
deposits are forbidden")) so it matches the updated deposit_asset implementation
and not the old AssetNotFound/MatchError path.
---
Nitpick comments:
In `@pallets/moonbeam-foreign-assets/src/lib.rs`:
- Around line 817-848: Replace the current if/matches check on asset_status with
an exhaustive match over AssetStatus (after the existing early-return for
AssetStatus::FrozenXcmDepositForbidden and after resolving beneficiary) so each
known variant is handled explicitly: handle AssetStatus::FrozenXcmDepositAllowed
by computing total_pending, inserting PendingDeposits::<T>::insert(...) and
emitting Event::PendingDepositRecorded, handle the Active/expected minting
variant by performing the storage-layer
EvmCaller::<T>::erc20_mint_into(contract_address, beneficiary, amount) call, and
add a catch-all arm that returns a clear XcmError (or MatchError) for any
unknown future variant; reference match_asset, asset_status, PendingDeposits,
beneficiary and EvmCaller::erc20_mint_into when making these changes so the
compiler will force exhaustiveness on AssetStatus.
In `@pallets/moonbeam-foreign-assets/src/tests.rs`:
- Around line 612-688: The test claim_pending_deposit_success should also assert
the emitted events to catch regressions: after the deposit call
(TransactAsset::deposit_asset) assert that PendingDepositRecorded (from the
EvmForeignAssets pallet events) was emitted with the expected asset id,
beneficiary and amount, and after claim_pending_deposit assert that
PendingDepositClaimed was emitted with the expected asset id, beneficiary and
amount; add similar PendingDepositRecorded assertions to
xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_asset and
pending_deposits_accumulate tests to ensure events are emitted alongside the
storage and balance checks (look for EvmForeignAssets::pending_deposits,
claim_pending_deposit, and the pallet event variants
PendingDepositRecorded/PendingDepositClaimed to locate the right spots).
In
`@test/suites/dev/moonbase/test-assets/test-foreign-assets-xcm-pending-deposits.ts`:
- Around line 127-130: The test currently calls .toJSON() on the SCALE-decoded
U256 and coerces it via BigInt(pendingDeposit as string), which is fragile;
instead obtain a typed codec or use the codec's native accessor on the returned
value from context.polkadotJs().query.evmForeignAssets.pendingDeposits(assetId,
beneficiary) (e.g. call .toBigInt() or .toString() on the codec instance) and
compare that result to depositAmount; update the pendingDeposit assignment and
the expect assertion to use the codec's .toBigInt() (or a strongly typed query
result) rather than BigInt(... as string) to avoid encoding breakage.
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.
Summary
allow_xcm_deposit = true(FrozenXcmDepositAllowed), incoming XCM deposits are no longer rejected — instead they are tracked in a newPendingDepositsstoragemap, keyed by (
asset_id,beneficiary), with amounts accumulating viachecked_addclaim_pending_depositextrinsic to mint the accrued tokens to the beneficiary via the ERC-20 contractallow_xcm_deposit = false(FrozenXcmDepositForbidden) continue to reject deposits as beforeChanges
pallets/moonbeam-foreign-assets/src/lib.rsPendingDepositsStorageDoubleMap<AssetId, H160, U256>claim_pending_depositextrinsic (call index 4) — requires asset to beActive, takes pending amount, mints via EVM, emitsPendingDepositClaimeddeposit_assetto store pending deposits when status isFrozenXcmDepositAllowedinstead of calling the EVM, emittingPendingDepositRecordedForeignAssetsMatcher::match_assetreturn type to includeAssetIdNoPendingDeposit,AssetNotActivePendingDepositRecorded,PendingDepositClaimedpallets/moonbeam-foreign-assets/src/tests.rs— 6 new tests:xcm_deposit_succeeds_on_frozen_xcm_deposit_allowed_assetpending_deposits_accumulateclaim_pending_deposit_successclaim_pending_deposit_fails_when_asset_frozenclaim_pending_deposit_fails_when_no_pendingxcm_deposit_blocked_on_frozen_xcm_deposit_forbidden_assetpallets/moonbeam-foreign-assets/src/benchmarks.rs— newclaim_pending_depositbenchmarkWeights — updated for all three runtimes (
moonbase,moonbeam,moonriver) and the pallet defaultTest plan
cargo test -p pallet-moonbeam-foreign-assets)FrozenXcmDepositAllowedasset records a pending deposit (no EVM mint)asset_id,beneficiary)claim_pending_depositmints tokens and clears storage after unfreezeclaim_pending_depositfails withAssetNotActivewhile asset is frozenclaim_pending_depositfails withNoPendingDepositwhen no deposit existsFrozenXcmDepositForbiddenstill rejects deposits outright