refactor(contracts): drop the dead ERC20 test doubles; pin the storage NFT facade#314
Merged
Conversation
puneet2019
force-pushed
the
refactor/slim-contracts
branch
from
25783ae to
fac7a26
Compare
…e NFT facade The four ERC20 test doubles (MinterBurnerDecimals, Burnable, DirectBalanceManipulation, MaliciousDelayed) have had zero references since the x/erc20 removal (#221); three of the four ship upstream in cosmos/evm should they ever be needed again. Delete them with their .sol sources, compiled artifacts, and the contracts npm manifest. contracts/ itself stays: cosmos/evm files module-consumed artifacts under a single top-level contracts/ package (x/erc20 and x/ibc/callbacks import from there), and moca follows that convention. What remains is the moca-specific ERC721NonTransferable facade the storage keeper mints/burns bucket/object/ group NFTs through — now pinned by a guard test covering the ABI surface the keeper calls (mint/burn), the six fixed token/hub addresses, and the fact that the artifact intentionally carries no bytecode: nothing deploys it and no precompile serves 0x3000-3002; the keeper only encodes calldata. The existing storage keeper tests (CreateBucket et al.) exercise the mint path and CI's Comprehensive e2e covers storage on a live chain. Lint wiring follows the deletions: Slither retargets contracts/ -> solidity/contracts/ (dropping the openzeppelin npm step the interfaces never needed), the contracts/-scoped solhint workflow is removed with its subject (solidity/'s own lint:sol currently fails on pre-existing natspec debt under --max-warnings 0, so wiring it into CI now would add a permanently red job), and the broken lint-contracts Make targets are dropped. Signed-off-by: puneetmahajan <59960662+puneet2019@users.noreply.github.com>
puneet2019
force-pushed
the
refactor/slim-contracts
branch
from
fac7a26 to
7ec76d4
Compare
puneet2019
changed the title
phenix3443
reviewed
Jul 7, 2026
phenix3443
left a comment
phenix3443
left a comment
Contributor
There was a problem hiding this comment.
Left two follow-ups: one on the stale contracts-compile developer target, and one on the new ABI guard test not asserting the exact method signatures it says it pins. I also re-ran go test ./contracts and go test ./x/storage/keeper locally while checking these paths.
…mpile targets Review follow-ups (phenix): The guard test asserted method presence and argument count only, so a reordered signature like mint(uint256,address) would still pass while breaking the keeper's positional CallEVM arguments. Assert the exact signatures (mint(address,uint256), burn(uint256)) instead; verified the reordered variant now fails the test. Remove the whole 'Compile Solidity Contracts' Makefile section (contracts-compile / openzeppelin / contracts-clean / create-contracts-json): it compiled the deleted erc20 .sol sources — its own comment says 'for the erc20 module' — and could no longer work without contracts/package.json. Worse, contracts-clean ran rm -rf contracts/compiled_contracts, which would now delete the kept ERC721 artifact that has no in-tree source to regenerate from. Signed-off-by: puneetmahajan <59960662+puneet2019@users.noreply.github.com>
…story Make the package answer the reviewer's question where a maintainer will actually look: the artifacts are ABI-only with no in-tree Solidity source (and never had one — the removed compile targets only ever built the deleted erc20 .sols), the embedded JSON is the source of truth, and changes go through editing the JSON plus the guard test that pins signatures, addresses, and the bytecode-less nature. Signed-off-by: puneetmahajan <59960662+puneet2019@users.noreply.github.com>
puneet2019
force-pushed
the
refactor/slim-contracts
branch
from
64276ac to
43f5b44
Compare
phenix3443
approved these changes
Jul 8, 2026
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.
Part of the shrink-to-upstream campaign (#309–#312). Slims
contracts/to what moca actually uses — the folder stays, matching cosmos/evm's own convention of a single top-levelcontracts/package for module-consumed artifacts (theirx/erc20andx/ibc/callbackskeepers import from top-levelcontracts/).Summary
MinterBurnerDecimals,Burnable,DirectBalanceManipulation,MaliciousDelayed) +.solsources + compiled JSONs + npm manifestx/erc20removal (#221) — verified symbol-by-symbol repo-wide. 3 of 4 ship upstream in cosmos/evm (contracts/solidity/...) if ever needed againERC721NonTransferablefacade + embedded artifactcontracts/(untouched; consumers untouched)x/evm/precompiles(which are callee-side Go implementations registered at addresses)Tests (regressions + integration)
contracts/erc721NonTransferable_test.go): pins the ABI surface the storage keeper actually calls (mint(address,uint256),burn(uint256)), the six fixed token/hub addresses (protocol constants), and — surfaced by the test — that the artifact intentionally carries no bytecode: it is an ABI-only facade. Nothing deploys it and no precompile is registered at0x…3000–3002(verified against the static-precompile map and repo-wide); the keeper only encodes calldata against fixed addresses.x/storage/keepertests (bucket_limit_testet al.) driveCreateBucketthrough the NFT mint path in-process; CI'sE2E: Comprehensiveexercises storage on a live chain.go build ./...;go test -raceovercontracts/,x/storage/keeper, and the storage precompile — locally and on an amd64 box (CI's arch).Lint wiring (had to follow the deletions)
target: contracts/→solidity/contracts/; diff-gate**/*.sol→solidity/**/*.sol; dropped thecd contracts && npm i+ openzeppelin copy step (thesolidity/interfaces import no OZ — verified; andcontracts/no longer has an npm manifest).contracts/**/*.sol, all deleted here. Not retargeted tosolidity/on purpose:solidity/'s ownnpm run lint:solcurrently fails on pre-existing natspec debt under--max-warnings 0(verified in docker) — wiring that into CI would add a permanently red job. The team can adopt it deliberately once the debt is paid.lint-contracts/lint-fix-contractsMake targets removed (they invoked npm scripts that never existed).Compile Solidity ContractsMakefile section (contracts-compile/openzeppelin/contracts-clean/create-contracts-json) is also removed — it compiled the deleted erc20.solsources (its own comment: "for the erc20 module") and could no longer work; worse,contracts-cleanranrm -rf contracts/compiled_contracts, which would now delete the kept ERC721 artifact (which has no in-tree source to regenerate from). The guard test now pins exact ABI signatures (mint(address,uint256),burn(uint256)) — verified to fail on a reordered signature — instead of arity only.🤖 Generated with Claude Code