refactor!: align precompile reverts with Solidity errors by nowooj · Pull Request #1099 · cosmos/evm

nowooj · 2026-04-03T07:43:05Z

Description

Precompile failures no longer rely primarily on string reasons. Failures are now returned as Solidity custom errors encoded from each module’s abi.json, via NewRevertWithSolidityError and ReturnRevertError in precompiles/common, including interpreter return data where applicable.

The ERC-20 precompile drops the split IERC20 / IERC20Metadata setup in favor of ERC20I, combining OpenZeppelin IERC20Metadata, IERC6093 IERC20Errors, and the shared IPrecompile interface. Other precompiles (bank, distribution, gov, ics02, ics20, slashing, staking, etc.) were updated for interfaces, errors.go, and regenerated abi.json.

Tooling: Solidity 0.8.26, Hardhat evmVersion: cancun, and OpenZeppelin v5, with contracts recompiled and artifacts aligned to Hardhat 3 (hh3-artifact-1).

Tests: Go integration tests and Solidity suites (revert_cases, ERC20) assert custom error selectors / revert data where relevant.

Review focus: precompiles/common/revert.go, use of NewRevertWithSolidityError in each module’s errors.go / tx.go / query.go, and consistency between ERC20I.sol and generated abi.json.

Breaking change: Call sites that assumed only Error(string) reverts may need to decode standard custom errors instead.

Closes: #954

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

tackled an existing issue or discussed with a team member
left instructions on how to review the changes (no change)
targeted the main branch

nowooj · 2026-04-03T07:50:45Z

Rebase on main

vladjdk · 2026-04-24T17:47:04Z

@greptile review

greptile-apps · 2026-04-24T17:52:57Z

Greptile Summary

This PR replaces string-based precompile reverts with ABI-encoded Solidity custom errors across all modules, introducing NewRevertWithSolidityError / ReturnRevertError in precompiles/common and regenerating abi.json artifacts throughout. The RevertError type in x/vm/types is refactored to store raw bytes instead of a hex string, and a new RevertData() accessor enables structured revert propagation from PostTxProcessing hooks.

P1 — dead code in state_transition.go: err.(types.RevertError) should be err.(*types.RevertError); because NewExecErrorWithReason returns a pointer, the assertion always fails and res.Ret is never populated with structured revert data from post-tx hooks.
P1 — wrong error in erc20/tx.go transfer: after msgSrv.Send fails, a post-hoc SpendableCoin check is used to guess whether the cause was insufficient balance; this heuristic fires the wrong ERC20InsufficientBalance custom error when sends fail for unrelated reasons (blocked address, sends disabled) but balance happens to be below the requested amount.

Confidence Score: 3/5

Two P1 bugs should be fixed before merging: a dead-code type assertion that silently drops structured revert data from PostTxProcessing hooks, and a wrong-error classification in the ERC20 transfer path.

The overall refactor direction is correct and most of the Solidity error wiring is verified to match abi.json. However, the value-type assertion in state_transition.go means the entire new structured-revert pathway for post-tx hooks is never exercised, and the ERC20 transfer balance-check heuristic can return an incorrect custom error selector under edge-case bank failures. Both issues affect correctness of the primary feature introduced by this PR.

x/vm/keeper/state_transition.go (wrong type assertion) and precompiles/erc20/tx.go (balance heuristic in transfer)

Important Files Changed

Filename	Overview
x/vm/keeper/state_transition.go	Type assertion err.(types.RevertError) uses the non-pointer concrete type while NewExecErrorWithReason returns *RevertError; the new PostTxProcessing revert-data pathway is permanently dead.
precompiles/erc20/tx.go	Post-hoc SpendableCoin balance check after Send failure can return ERC20InsufficientBalance for unrelated errors (blocked account, sends disabled, etc.).
precompiles/common/revert.go	New RevertWithData + NewRevertWithSolidityError infrastructure is sound, but pack failures silently degrade to Error(string) with ABI diagnostics.
x/vm/types/errors.go	RevertError refactored to store raw bytes instead of hex string; ErrorData() output is semantically equivalent; new RevertData() accessor added correctly.
precompiles/erc20/errors.go	ConvertErrToERC20Error removed; ERC-6093-aligned Solidity error name constants introduced; cleaner than the old string-matching approach.
precompiles/staking/tx.go	All error paths migrated to Solidity custom errors; CannotCallFromContract ABI signature matches (address, uint256, bool).
precompiles/distribution/tx.go	Consistent migration to Solidity custom errors; method-name labels added as typed constants; no ABI signature mismatches detected.

Sequence Diagram

sequenceDiagram
    participant Caller as EVM Caller
    participant Precompile
    participant Revert as precompiles/common/revert.go
    participant EVMT as x/vm/types.RevertError
    participant Interp as vm.Interpreter

    Caller->>Precompile: opCall → Execute()
    alt Solidity custom error path
        Precompile->>Revert: NewRevertWithSolidityError(abi, errName, args...)
        Revert-->>Precompile: *RevertWithData{data: selector+packed}
        Precompile->>Revert: ReturnRevertError(evm, *RevertWithData)
        Revert->>Interp: SetReturnData(data)
        Revert-->>Caller: (data, ErrExecutionReverted)
    else String fallback path
        Precompile->>Revert: ReturnRevertError(evm, plainErr)
        Revert->>EVMT: RevertReasonBytes(err.Error())
        EVMT-->>Revert: Error(string) ABI bytes
        Revert->>Interp: SetReturnData(bytes)
        Revert-->>Caller: (bytes, ErrExecutionReverted)
    end

    note over Caller,EVMT: PostTxProcessing hook (state_transition.go)
    Caller->>Precompile: PostTxProcessing hook returns *RevertError
    Precompile->>EVMT: err.(*types.RevertError) — MUST be pointer assertion
    note right of EVMT: Bug: err.(types.RevertError) always false

_{Reviews (1): Last reviewed commit: "tests: enhance error handling with speci..." | Re-trigger Greptile}

greptile-apps · 2026-04-24T17:53:01Z

+			if rdp, ok := err.(types.RevertError); ok {
+				res.VmError = vm.ErrExecutionReverted.Error()
+				res.Ret = rdp.RevertData()
+			} else {
+				res.VmError = errorsmod.Wrap(err, "failed to execute post transaction processing").Error()


Wrong type assertion — new revert pathway never fires

types.RevertError is a struct type, and NewExecErrorWithReason returns *types.RevertError (pointer receiver). In Go, a concrete type assertion err.(types.RevertError) only succeeds when the dynamic type is the exact non-pointer value type. Since every hook that returns a RevertError does so via the pointer (&RevertError{...}), this assertion always evaluates to ok = false, the else branch always runs, and res.Ret is never populated with structured revert data — the whole new structured pathway is silently dead code.

Suggested change

if rdp, ok := err.(types.RevertError); ok {

res.VmError = vm.ErrExecutionReverted.Error()

res.Ret = rdp.RevertData()

} else {

res.VmError = errorsmod.Wrap(err, "failed to execute post transaction processing").Error()

if err != nil {

// If hooks returns an error, revert the whole tx.

if rdp, ok := err.(*types.RevertError); ok {

greptile-apps · 2026-04-24T17:53:02Z

 	msgSrv := NewMsgServerImpl(p.BankKeeper)
 	if err = msgSrv.Send(ctx, msg); err != nil {
-		// This should return an error to avoid the contract from being executed and an event being emitted
-		return nil, ConvertErrToERC20Error(err)
+		spendable := p.BankKeeper.SpendableCoin(ctx, from.Bytes(), p.tokenPair.Denom)
+		bal := spendable.Amount.BigInt()
+		if amount.Cmp(bal) > 0 {
+			return nil, cmn.NewRevertWithSolidityError(p.ABI, SolidityErrERC20InsufficientBalance, from, bal, amount)
+		}
+		return nil, cmn.NewRevertWithSolidityError(p.ABI, cmn.SolidityErrMsgServerFailed, method.Name, err.Error())
 	}


Post-hoc balance heuristic returns wrong error for non-balance send failures

After msgSrv.Send fails, the code checks whether amount > SpendableCoin to decide whether to return ERC20InsufficientBalance. This heuristic can mis-classify errors: if the send fails for a reason unrelated to balance (e.g., sends disabled for the denom, a blocked recipient, module account restrictions), but the sender's balance also happens to be less than the requested amount, ERC20InsufficientBalance is returned with incorrect balance and needed fields instead of the real underlying error. The previous ConvertErrToERC20Error at least inspected the actual error message; this approach ignores the actual failure cause entirely.

greptile-apps · 2026-04-24T17:53:03Z

+		return &RevertWithData{data: revertReasonBz}
+	}
+
+	data, err := customErr.Inputs.Pack(args...)
+	if err != nil {
+		return err
+	}
+	return &RevertWithData{data: append(customErr.ID[:4], data...)}
+}
+


ABI-pack failures silently fall back to Error(string) with an ABI error message

When customErr.Inputs.Pack(args...) fails (e.g., a caller passes the wrong number or type of arguments), NewRevertWithSolidityError returns the raw pack error directly — not a RevertWithData. ReturnRevertError then encodes err.Error() (the pack diagnostic) as an Error(string) revert, so the on-chain revert data will be an ABI encoding error string rather than the intended custom error. There is no log, no panic, and no compile-time guard, making mismatches invisible until a unit test catches them.

vladjdk · 2026-04-24T18:05:11Z

@nowooj a few conflicts here

vladjdk

Few greptile fixes required and conflicts need fixing.

…Error

aljo242

ci has not run: all workflows are action_required -- fork pr, needs maintainer approval to trigger.

no abi regeneration hook: abi.json artifacts are manually edited. if solidity source drifts from its abi.json, the custom error selector will be silently wrong at runtime (falls back to string error with no build-time signal). add a make generate-abis or equivalent target and run it in ci.

unverified ReturnRevertError wrapping: several helpers (bech32/methods.go, distribution/query.go, etc.) return NewRevertWithSolidityError directly. confirm all call sites at the precompile Run() boundary wrap them in ReturnRevertError -- an unwrapped return leaks the raw revert bytes as a Go error instead of EVM revert data.

missing changelog entry: this is a breaking change (callers decoding Error(string) reverts now receive 4-byte selector custom errors). needs a breaking changes entry in CHANGELOG.md.

nowooj · 2026-05-29T08:04:00Z

ci has not run: all workflows are action_required -- fork pr, needs maintainer approval to trigger.

no abi regeneration hook: abi.json artifacts are manually edited. if solidity source drifts from its abi.json, the custom error selector will be silently wrong at runtime (falls back to string error with no build-time signal). add a make generate-abis or equivalent target and run it in ci.

unverified ReturnRevertError wrapping: several helpers (bech32/methods.go, distribution/query.go, etc.) return NewRevertWithSolidityError directly. confirm all call sites at the precompile Run() boundary wrap them in ReturnRevertError -- an unwrapped return leaks the raw revert bytes as a Go error instead of EVM revert data.

missing changelog entry: this is a breaking change (callers decoding Error(string) reverts now receive 4-byte selector custom errors). needs a breaking changes entry in CHANGELOG.md.

Added a Precompile ABI Check workflow that runs make contracts-compile and checks git diff --exit-code -- ':(glob)precompiles/**/abi.json'.

For the ReturnRevertError concern, I added Solidity coverage in the precompile suite instead of a static boundary test. The tests now exercise the relevant precompile revert paths and decode the returned revert data with each precompile interface via iface.parseError(getRevertData(error)). This covers staking, distribution, bech32, ERC20, ICS02, ICS20, slashing, gov, and WERC20 custom-error paths. Bank’s ABI-valid query methods do not currently expose a custom-error revert path from Solidity, so I left that as-is rather than adding an artificial malformed-call test..

nowooj force-pushed the refactor/sol-error branch from 0c7c32b to 2e1567f Compare April 3, 2026 08:55

vladjdk requested a review from a team as a code owner April 23, 2026 20:25

greptile-apps Bot reviewed Apr 24, 2026

View reviewed changes

vladjdk requested changes Apr 27, 2026

View reviewed changes

nowooj force-pushed the refactor/sol-error branch from 2e1567f to b29db74 Compare May 6, 2026 09:15

nowooj added 10 commits May 22, 2026 10:43

refactor: use solidity error

cb2bf58

chore: make contracts-compile

b6dd566

tests: enhance error handling with specific solidity error

3d845f2

feat: use openzepplin v5

d91178b

refactor: erc20 use ERC20I and improve error handling

c7d3fcd

chore: make contracts-compile

805b541

tests: enhance error handling with specific solidity error in erc20

18f5f25

fix: error handling in ApplyTransaction by using errors.As for Revert…

e9fc6b7

…Error

fix: improve error handling for insufficient funds in ERC20 transfer

f3ab669

fix: unknown solidity custom errors and packing failures

5ce5acd

nowooj force-pushed the refactor/sol-error branch from 2346ec2 to 5ce5acd Compare May 22, 2026 01:52

aljo242 requested changes May 26, 2026

View reviewed changes

nowooj and others added 3 commits May 29, 2026 14:34

Merge branch 'main' into refactor/sol-error

65274bc

chore: update CHANGELOG and add Precompile ABI Check workflow

4640346

feat: add custom error handling for precompiles and enhance tests

5bf4b63

nowooj changed the title ~~refactor: align precompile reverts with Solidity errors~~ refactor!: align precompile reverts with Solidity errors May 29, 2026

Conversation

nowooj commented Apr 3, 2026

Description

Author Checklist

Uh oh!

nowooj commented Apr 3, 2026

Uh oh!

vladjdk commented Apr 24, 2026

Uh oh!

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

nowooj May 6, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

nowooj May 6, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

nowooj May 6, 2026

Choose a reason for hiding this comment

Uh oh!

vladjdk commented Apr 24, 2026

Uh oh!

vladjdk left a comment

Choose a reason for hiding this comment

Uh oh!

aljo242 left a comment

Choose a reason for hiding this comment

Uh oh!

nowooj commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants