Remove the EVM pointer creation path - #4230
alexander-sei wants to merge 7 commits into
Conversation
An EVM pointer is an ERC20 for a native denom, or an ERC20/721/1155 for a CW20/CW721/CW1155 contract. Three routes could create one: the pointer precompile at 0x...100b, the AddERCNativePointerProposalV2 governance proposal, and the seid commands driving them. All three are closed here. The precompile keeps its address and ABI so the four add* selectors still decode and revert with an Error(string) reason, following the retirement already applied to the ibc and oracle precompiles. Unregistering the address would instead let a call succeed against empty code and return nothing, failing a caller silently rather than loudly. All eight pointer governance proposals refuse submission and execution but stay registered as decode-only content. Gov never prunes a proposal once its vote ends, MustUnmarshalProposal unpacks the content Any on every read, and Router.GetRoute panics on a route it cannot resolve, so dropping either the registration or the evm route would fault a node reading or tallying an old proposal. The evm route now points at a handler that fails, mirroring app/retiredibc/gov. The giga/deps/xevm copies were never wired into gov and are deleted outright. Migration boundaries 8 and 17 join 4, 6, 7, 9 and 15 as inert: their only body was pointer creation, and a module's migration sequence must be unbroken for a chain that upgrades across it. Keeper.UpsertERCPointer and the pointer bytecode under artifacts/ are retained because precompiles/pointer/legacy/* replays historical creation under debug_trace*. Nothing live reaches them. The read side is untouched: registry and reverse-registry lookups, the pointerview precompile, and the pointer, pointer-version and pointee queries all keep serving pointers already deployed on chain. No integration test can create a pointer any more, so the four ERC*Pointer hardhat suites and the pointer-backed dapp and rpc scenarios are gone, and pointerview.spec.ts now covers unregistered lookups only. A co-located CosmWasm transfer produces a shell receipt only when its contract has a pointer, so the rpc fixtures assert the cumulative-gas series as an exact running sum rather than tolerating a jump. Co-authored-by: Cursor <cursoragent@cursor.com>
#4213 removed the precompile snapshots older than v6.6, so a traced height below the v6.6 upgrade no longer resolves to a snapshot that creates pointers - it resolves to the live precompile, which this branch retires. Three pacific-1 regression fixtures trace pointer creation at such heights and now report the retirement instead of the deploy. 0x4d9601... and 0x99d895... already expected an error and only move gas. 0xd09db4... flips: it expected success returning the new pointer address and now traces as a revert carrying the Error(string) retirement reason, which is pinned in full the same way #4213 pinned the retired oracle payload. This extends the trade-off #4213 documented by one step. It does not reach any height at or after the v6.6 upgrade, where the retained v6.6 snapshot still replays creation faithfully. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview The pointer precompile at Solidity and CI cleanup drops the pointer contract sources, their Foundry/Hardhat tests, pointer helpers in Docs/examples point local Hardhat testing at Reviewed by Cursor Bugbot for commit e67a9c1. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0e091b8. Configure here.
| it('block.gasUsed equals Σ receipt gasUsed and cumulativeGasUsed is consistent', async () => { | ||
| const block = await byHash(sei, richSei.hash, true); | ||
| await assertGasAccounting(sei, block, richSei.cosmosShellGas); | ||
| await assertGasAccounting(sei, block); |
There was a problem hiding this comment.
Gas checks drop Cosmos contribution
Medium Severity
Rich-block gas assertions now equate block.gasUsed to the EVM receipt sum and no longer pass cosmosShellGas. buildRichSeiBlock still co-locates a Cosmos CW20 transfer, and those non-EVM txs contribute to block.gasUsed, so the equality fails or flakes on dual-VM blocks.
Additional Locations (2)
Triggered by learned rule: test: Sei RPC block-level assertions must account for non-EVM txs in dual-VM blocks
Reviewed by Cursor Bugbot for commit 0e091b8. Configure here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4230 +/- ##
==========================================
- Coverage 66.91% 65.89% -1.02%
==========================================
Files 2176 2056 -120
Lines 167167 154972 -12195
==========================================
- Hits 111858 102121 -9737
+ Misses 55168 52710 -2458
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
IBC.sol, Oracle.sol and Pointer.sol are reference interfaces published alongside their precompiles for contract authors to import. All three precompiles now refuse every call, so the interfaces describe an API that cannot be exercised: a contract written against one compiles and then reverts at runtime. Nothing in the tree imports them and no script, doc or CI job names them. The interfaces of the seventeen live precompiles are untouched, as is every Solidity source under contracts/src - CW20ERC20Pointer.sol and WSEI.sol have no importer either, but solc builds the bytecode embedded in x/evm/artifacts from them. Co-authored-by: Cursor <cursoragent@cursor.com>
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The retirement itself is well-executed and faithfully follows the existing precompiles/ibc/retiredibc tombstone patterns, with the gov-registry and migration-sequence reasoning correct. Two blockers remain: CI still invokes a deleted test script, and the new retirement integration test asserts an error string the precompile never produces.
Findings: 2 blocking | 2 non-blocking | 1 posted inline
Blockers
.github/workflows/integration-test-matrix.jsonstill lists./integration_test/evm_module/scripts/evm_interoperability_pointer_tests.shat lines 54 and 75 (rows "EVM Interoperability (Pointer Tests)" and "Autobahn EVM Interoperability (Pointer Tests)"), but the PR deletes that script. Both matrix jobs will fail immediately with "No such file or directory". Since the script's entire body was the four deletedERC*Pointerhardhat suites, both rows should be removed from the matrix along with it.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
contracts/README.md:5still documentsnpx hardhat test --network seilocal test/ERC20toCW20PointerTest.js, which this PR deletes. Point the example at a suite that still exists. - 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
contracts/test/lib.js:525proposeCW20toERC20Upgradeshells out toseid tx evm add-cw-erc20-pointer, a CLI command that no longer exists (removed with the CW-wrapper direction). It is exported but has no callers, so it is dead code that would fail if reused.
…emoval Two rows in .github/workflows/integration-test-matrix.json, "EVM Interoperability (Pointer Tests)" and its Autobahn twin, ran the evm_interoperability_pointer_tests.sh script this branch deletes; both jobs would have failed on a missing file. The script's whole body was the four ERC*Pointer hardhat suites that went with the creation path, so the rows go with it. pointer.spec.ts asserted the VmError contained "EVM pointer creation is deprecated", a string from an earlier draft of the precompile. It reverts with ErrPointerPrecompileRetired, "pointer precompile is retired; EVM pointer creation is disabled", so the assertion could never hold. It now matches on the retirement text the way oracle.spec.ts does. contracts/README.md pointed its worked example at a deleted suite, and lib.js kept proposeCW20toERC20Upgrade, which drives the add-cw-erc20-pointer command that went away with the CosmWasm wrapper direction in #4198. It had no callers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both seidroid blockers were real and are fixed in c19b3f0. The matrix rows are gone — On Bugbot's "Gas checks drop Cosmos contribution" — I believe that one is a false positive, and I'd rather leave it here than have someone re-add the tolerance later. A co-located CosmWasm tx only gets a shell receipt when its contract has a registered pointer: |
Nothing reads these. x/evm/artifacts embeds the checked-in .abi and .bin, so the Go build never touches a .sol, and the pointer bytecode is frozen: it can never be redeployed, so a rebuild could only produce something that differs from what is on chain. Deleting the four pointer sources takes their three Foundry tests with them, since those import them, and that orphans the four interfaces under contracts/src/precompiles, which nothing else imported. They were the repo's only .t.sol files, so forge-test.yml would have built and tested an empty project and goes too. foundry.toml and contracts/lib stay: five sources under contracts/src still resolve @OpenZeppelin through them, and forge build remains the documented way to produce artifacts. x/evm/artifacts/README described regenerating the artifacts with solc from sources that no longer exist; it now records that they are frozen and why. The Foundry and hardhat instructions in contracts/README.md lose the pointer rebuild section, and example/contracts/README loses its pointer solc line. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
The pointer retirement is thorough and internally consistent — precompile tombstone, gov proposal/route retention, inert migration boundaries, CLI removal and fixture updates all line up, and both previously reported blockers are fixed. Only non-blocking notes remain: two new tests assert less than their names claim, and the revert-reason encoder is duplicated a third time.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] integration_test/dapp_tests/nftMarketplace/nftMarketplaceTests.js: inside
testNFTMarketplaceOrder,nftOwneris declared withletinside theif (seller.address !== deployer.address)block but reassigned after it (nftOwner = await nftContract.ownerOf(tokenId)), creating an implicit global. It works in CommonJS sloppy mode but would throw under strict mode/ESM. Declaring it at function scope fixes it.
| false, | ||
| false, | ||
| ) | ||
| require.ErrorIs(t, err, vm.ErrExecutionReverted) |
There was a problem hiding this comment.
[suggestion] This test passes whether or not ValidateNonPayable is still called: every call to the retired precompile returns vm.ErrExecutionReverted, so removing the non-payable guard would not fail TestRetiredPrecompileRejectsValue.
ValidateNonPayable returns errors.New("sending funds to a non-payable function") (precompiles/common/precompiles.go:261) and short-circuits before the retirement error, so the two are distinguishable. Asserting on statedb.GetPrecompileError() — that it contains the non-payable message and is not pointer.ErrPointerPrecompileRetired — makes the test actually pin the guard, mirroring how TestRetiredPrecompileRejectsCalls above asserts ErrorIs(statedb.GetPrecompileError(), pointer.ErrPointerPrecompileRetired).
| pointer.addCW20Pointer.staticCall(runtime.funded.adminSeiAddress), | ||
| 'pointer.addCW20Pointer with a non-contract address', | ||
| ); | ||
| it('the CW methods revert', async () => { |
There was a problem hiding this comment.
[suggestion] This case does not pin the retirement. expectExecutionReverted only matches /execution reverted|revert/i, and before the retirement addCW20Pointer/addCW721Pointer/addCW1155Pointer already reverted for a valid non-contract sei address (sdk.AccAddressFromBech32 succeeded, then QuerySmartSafe failed) — so this test would pass unchanged against a build where creation still works.
expectExecutionReverted returns the error message, so asserting it contains RETIRED (already defined at line 28) makes the three CW selectors as load-bearing as the addNativePointer case above. The same applies to the dispatch semantics loop at line 84: all three entrypoints revert regardless, so nothing there distinguishes the retirement from the pre-existing staticcall/delegatecall guards.
| return nil, 0, err | ||
| } | ||
| res, err := p.wasmdKeeper.QuerySmartSafe(ctx, cwAddress, []byte("{\"token_info\":{}}")) | ||
| func mustEncodeRevertReason(reason string) []byte { |
There was a problem hiding this comment.
[suggestion] mustEncodeRevertReason is now the third byte-identical copy in non-generated code — precompiles/oracle/oracle.go:115 and precompiles/ibc/ibc.go:69 have the same nine lines. Both files already import pcommon, so hoisting it there (e.g. pcommon.MustEncodeRevertReason) collapses all three; the copies under precompiles/*/legacy/* are frozen snapshots and should stay as they are.
Worth doing now rather than at the fourth retirement: the Error(string) selector and the argument packing are exactly the kind of detail that drifts between copies unnoticed, since each is exercised only by its own precompile's tests.
Superseded: latest AI review found no blocking issues.
1 participant
An EVM pointer is an ERC20 standing in for a native bank denom, or an ERC20/721/1155 standing in for a CW20/CW721/CW1155 contract. Three routes could create one: the pointer precompile at
0x…100b, theAddERCNativePointerProposalV2governance proposal, and theseid tx evm register-evm-pointerandadd-erc-native-pointercommands driving them. #4198 closed the opposite direction, CosmWasm wrappers for ERC tokens; this closes the remaining one, so no pointer of any kind can be created from here.The precompile keeps its address and ABI, and every call now returns
ErrPointerPrecompileRetiredwith anError(string)payload, followingprecompiles/ibcandprecompiles/oracle. Unregistering0x…100bwas the alternative and is worse: a call to an address with no code succeeds and returns nothing, soIPointer(…).addNativePointer(…)would start failing silently instead of reverting.x/evm/gov.go,NewProposalHandlerand the two CLI commands are gone, and migration boundaries 8 and 17 join 4, 6, 7, 9 and 15 as inertRetiredPointerBoundaryentries, since a module's migration sequence has to stay unbroken for a chain upgrading across it.The eight
Add*Pointer*Proposaltypes are retired rather than deleted, which is the part worth scrutinising. Gov never prunes a proposal once its vote ends,MustUnmarshalProposalunpacks the contentAnyon every read, andRouter.GetRoutepanics on a route it cannot resolve — so dropping the registration would panic a node reading an old proposal, and dropping theevmroute would panic one still in the voting queue. Both stay, andValidateBasic,ValidateProposalSubmissionand the route handler all refuse withErrPointerProposalDeprecated. Thegiga/deps/xevmcopies were never wired into gov and are deleted outright.Keeper.UpsertERCPointerand the bytecode underx/evm/artifacts/stay, becauseprecompiles/pointer/legacy/v66still needs them to replay historical creation underdebug_trace*.The Solidity side goes with the capability.
Pointer.sol,IBC.solandOracle.solare the published interfaces of the three precompiles that now refuse every call, so a contract author cannot compile against an API that only reverts. The four pointer sources undercontracts/srcgo too: the Go build embeds the checked-in.abiand.binand never reads a.sol, and the bytecode is frozen, so a rebuild could only produce something that differs from what is deployed. That takes their three Foundry tests, which import them, and orphans the four interfaces undercontracts/src/precompiles— and since those were the repo's only.t.solfiles,forge-test.ymlwould have tested an empty project and is removed.foundry.tomlandcontracts/libstay, because five remaining sources resolve@openzeppelinthrough them.x/evm/artifacts/READMEdescribed regenerating the artifacts from sources that no longer exist and now records that they are frozen instead.Consensus at current height is unaffected and the read path is untouched: registry and reverse-registry lookups, the
pointerviewprecompile, and thepointer,pointer-versionandpointeequeries all keep serving pointers already deployed on chain. Threeevmrpc/testsregression fixtures move, because #4213 removed the pre-v6.6 snapshots and pacific-1 pointer-creation traces below the v6.6 upgrade height now resolve to the live precompile and report the retirement rather than the deploy, one of them flipping from success to revert. Integration coverage that created pointers goes as well: the fourERC*Pointerhardhat suites and the twointegration-test-matrix.jsonrows that ran them, the pointer-backed dapp and rpc scenarios, andpointerview.spec.ts's registered-pointer case, which leaves that spec covering unregistered lookups only. Validated withgo build,go vet,make fmtcheck,golangci-lint run,tsc --noEmiton both TypeScript suites, andscripts/ramtest.shoverx/evm,precompiles,app,evmrpc,occ_tests,giga,wasmbindingandtestutil.