Fix benchmark build break and review nits on ERC-20 precompile transfers

[F-OBrien]

Stacked on top of #1984 (precompiles-fix), targeting that branch rather than develop.

The deferred-transfer fix in #1984 is correct — I traced Ok(None) back through
base_try_execute_instruction (pallets/settlement/src/lib.rs:3834) and
execute_instruction_retryable (:2003-2006) to confirm it genuinely means "tokens moved",
with no "executed but silently failed" case leaking through. Reverting before the Transfer
log is emitted is the right shape, and the allowance spend is inside the rolled-back scope,
so transferFrom no longer burns allowance on the deferred path.

This PR fixes a build break and applies review nits on top.

Blocking fix: benchmark builds don't compile

SettlementFnTrait::transfer_funds declares a 6th parameter under
#[cfg(feature = "runtime-benchmarks")] (primitives/src/traits/settlement.rs:75-82). Both new
call sites in erc20.rs passed 5 arguments. pallets/precompiles/Cargo.toml has a
runtime-benchmarks feature that enables polymesh-primitives/runtime-benchmarks, and all three
runtimes enable pallet-precompiles/runtime-benchmarks, so the cfgs are in lockstep.

Confirmed against cargo check -p polymesh-runtime-develop --features runtime-benchmarks:

error[E0061]: this function takes 6 arguments but 5 arguments were supplied
  --> pallets/precompiles/src/interface/erc20.rs:75:15
   | argument #6 of type `bool` is missing
error[E0061]: this function takes 6 arguments but 5 arguments were supplied
  --> pallets/precompiles/src/interface/erc20.rs:240:15

Fixed by passing #[cfg(feature = "runtime-benchmarks")] false at both sites, matching every
other call site in the workspace (pallets/nft/src/lib.rs:790, pallets/asset/src/lib.rs:2908,
and the base_transfer_asset call this replaced). The default build was always clean — this is
purely feature-gated, which raises a question: does CI run a --features runtime-benchmarks
job? If not, this class of break reaches develop unnoticed.

Nits

• Stale comment referring to base_transfer_asset, which is no longer called.
• if let Some(_) = inst_id → inst_id.is_some() (clippy redundant_pattern_matching).
• Dropped an unnecessary from.clone() — the earlier borrow has ended.
• Shortened the ERR_INST_NOT_EXECUTED revert string to a stable sentinel
  ("TransferDeferred: receiver affirmation or mediator required") and documented it. Revert
  data is returned to the caller on every failed call, so ~110 chars of prose is worth trimming.

Reviewed and deliberately left alone

The with_transaction wrapper in mod.rs — kept. It is redundant, but harmless, and
@HenriqueNogara mentioned test-based reasons for adding it, so it should not be removed without seeing
that test case. The analysis:

• Precompiles have exactly one invocation site: exec.rs:1379, inside Stack::run(). Every
  entry path (run_call at :861, nested calls at :939/:1915/:2131, instantiate at
  :2054) funnels through run().
• run() wraps its whole body — including that call — in with_transaction and rolls back on
  the REVERT flag (exec.rs:1456-1466). Instance::call sets that flag for both Error::Revert
  and Error::Panic (precompiles.rs:314-320); Error::Error propagates as Err and also hits
  the Rollback arm.
• Substrate events are plain storage writes (EventCount::put, Events::append,
  EventTopics::append in frame_system/src/lib.rs:1889-1918), so they are inside that same
  overlay transaction and are discarded too.

So the revert alone already undoes storage and substrate events. What the wrapper does not fix
is the one thing that genuinely is not rolled back: Ext::deposit_event also appends to the
environmental! AccumulateReceipt buffer (evm/block_storage.rs:47,124-128), which has no
transactional behaviour — a log emitted before a revert stays in the eth receipt and bloom no
matter how many with_transaction layers wrap it.

Best reconstruction: the "instruction persisted" behaviour predates the fix, because the
precompile used to return Ok on the deferred path — no revert, so no rollback. Both changes
landed in one commit, which would make the wrapper look load-bearing. @HenriqueNogara — if you have the
failing test, please share it; if it was receipt/log-based, the fix came from reordering the
emit after the check, not from the wrapper. Cost of keeping it is one overlay layer per call
(depth 25 x 2, against frame_support's limit of 255), so there's no urgency either way.

No gas refund on the error path. I initially suggested making adjust_gas symmetric on the
Err arm; that was wrong. The settlement pallet charges its benchmarked base weight at
pallets/settlement/src/lib.rs:1606-1609, after ensure_origin_call_permissions, the DID
lookups and the SenderSameAsReceiver check. A call failing early therefore has
weight_meter.consumed() == 0, and refunding to that would make failing transfers nearly free
while still performing real DB reads. The transfer_funds extrinsic doesn't refund on error
either. Current behaviour is correct.

transfer no longer emits CreatedAssetTransfer. Bypassing base_transfer_asset
(pallets/asset/src/lib.rs:2911-2918) means EVM-initiated transfers now emit FundsTransferred /
settlement events instead. Confirmed as intended — it makes transfer consistent with
transferFrom — but flagging it here so it's a recorded decision, and it's worth checking
against middleware/subquery before this reaches a public network.

Follow-ups, not in this PR

1. Tests. There is currently no test coverage for this precompile anywhere in
   pallets/runtime/tests/. Deliberately out of scope here, but it should not ship without at
   least: receiver with MandatoryReceiverAffirmation → transfer reverts, balances unchanged,
   no Transfer log, allowance untouched; asset with MandatoryMediators → same; happy path
   emits exactly one Transfer.
2. ExecutionMode::RequireImmediate in the settlement pallet. Today the deferred path does
   all the work — creates the instruction, affirms the sender leg, locks the funds, spends the
   allowance — and then throws it away. Every EVM transfer of a mediator-configured asset pays
   full weight to fail. A pre-check before instruction creation, using
   Asset::<T>::skip_asset_holder_affirmation and MandatoryMediators (the only two deferral
   sources today, pallets/settlement/src/lib.rs:1950-1957), would give both a cheap failure and
   a specific error. It would also move the invariant into the runtime so call_runtime/ink!
   callers get it too (F1 in allowance-transfer-funds-smart-contract-review.md). Needs a
   decision — it touches settlement, asset, nft and benchmarks.
3. Typed Solidity error. precompiles::Error only has Revert(Revert) (alloy's Revert is
   specifically Error(string)), Panic(PanicKind) and Error(ExecError). There is no variant
   carrying raw ABI-encoded revert data, so error TransferDeferred() cannot be returned without
   an upstream pallet-revive change. Worth raising upstream.
4. Upstream: sub-call reverts don't discard eth logs. See the AccumulateReceipt note above.
   A contract that calls this precompile inside try/catch and then succeeds produces a receipt
   with status 1 that still carries the phantom Transfer log. Worth an upstream report plus a
   regression test asserting eth_getTransactionReceipt logs are empty in that scenario.
5. Document known ERC-20 deviations. Pre-existing, but now more visible since these are the
   real transfer paths: zero-value transfers revert (ERC-20 says they MUST succeed and emit
   Transfer); self-transfers revert (SenderSameAsReceiver at :1596, SameSenderReceiver at
   :2890); contract callers need a DID, since transfer_funds runs
   ensure_origin_call_permissions against the contract's derived AccountId, which rules out
   most vault/router composability. Plus decimals() hardcoded to 6 and the unimplemented
   permit/nonces/DOMAIN_SEPARATOR.

🤖 Generated with Claude Code