Repository navigation
Remove the tokenfactory module behind the v6.8 upgrade - #4536
Conversation
Delete x/tokenfactory and proto/tokenfactory, unwire the module from the app, and delete the tokenfactory store and version-map entry at the v6.8 upgrade. The six Msg types move to a decode-only app/retiredtokenfactory package so historical transactions still decode; new ones are refused at CheckTx. Remove the factory/ denom transfer allowlist from the sei-cosmos and giga bank keepers and prune the stored allowlists at v6.8. Drop the tokenfactory wasm custom messages and query route, the tooling entries, and the tests and CI fixtures that created tokenfactory denoms. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4536 +/- ##
==========================================
+ Coverage 57.29% 57.39% +0.10%
==========================================
Files 2143 2125 -18
Lines 167425 166618 -807
==========================================
- Hits 95927 95636 -291
+ Misses 71493 70977 -516
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@seidroid review |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This removes the tokenfactory module behind the v6.8 upgrade. The store is deleted and its version-map entry dropped, the six Msg types stay decodable but are refused at validation (codespace tokenfactory, code 26), and the factory/ transfer allowlist is dropped from both bank keepers and pruned from the bank store in the handler. I found nothing blocking: the 0x11 prefix holds only allowlist keys, so the prune touches nothing else in the bank store; sign bytes, type URLs and fee-free refusal are covered by tests; and the one scout claim (codex, about the v6.0.2/v6.0.3 regression fixtures) was dropped because REVIEW.md §2 rules out a newer binary re-executing or tracing pre-upgrade heights, so the fixture shift is the accepted limitation the description names.
Non-blocking
- The pacific-1 regression fixtures changed here (0x29b9…, 0x534a…, and the unchanged 0xc2da…) show a CosmWasm contract minting
factory/sei1e3gttzq5…/iseithrough tokenfactory custom messages. If that contract is still live, its mint and burn flows will start reverting at the v6.8 height with no deprecation window, and the supply it backs will be frozen. Confirm its operators know before the upgrade is proposed.
seidroid review · decision approve · session 265dd04dce2a4eb8ae9eb29efa074f6c · turn resp_claude_e0cea4438970e083040428afdf1aaea5 · item 26e262db61d45a149c7564fac9fa8104
Findings: 0 blocking | 1 non-blocking | 0 posted inline
The v6.8 handler now also deletes the tokenfactory parameter subspace from the params store, so none of the module's state outlives it. The in-process test seeds the parameter v6.7 keeps and a creation fee an older version left behind, and the offline fixture adds the params store to its retained-store snapshot, so a v6.7-written database shows the prune touches nothing else. Drop the unused AllowList copy from giga's bank.pb.go. Its embedded descriptor is left as is: it registers under the same file name as the sei-cosmos descriptor, which still has to describe AllowList for the retired tokenfactory messages. Also correct the AllowList field comment, delete the factoryDenomPrefix constants only the removed allowlist tests used, and mark the 0xc2da regression fixture as a revert, which it has traced as since #4202. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview Decode-only compatibility lives in Bank / Giga no longer implement App-hash breaking at the v6.8 upgrade height. Reviewed by Cursor Bugbot for commit e964473. Bugbot is set up for automated code reviews on this repo. Configure here. |
Creating a tokenfactory denom allowlist costs only gas, and each one is a single bank key, even when the list is empty. The v6.8 handler deleted all of them in the upgrade block, so anyone could grow that work before the upgrade height. The delete held every key in memory and left every delete pending in the bank cache for the rest of the block, where each bank iterator a transaction opened had to scan them. deleteByPrefix, which every v6.8 prune goes through, now deletes at most RetiredPrefixDeleteLimit (1,000) keys in key order and leaves the rest. Nothing reads the allowlists after this change, so any that survive are inert. DenomAllowListPrefix is declared again in both bank key.go files, marked deprecated, so no later bank key reuses 0x11, and the app's private copy of the prefix is gone. TestV68DenomAllowListPruneIsBounded seeds one allowlist more than the limit and checks that only the last one survives and that it does not restrict a send of its denom. The CHANGELOG entry now says that denoms with an allowlist become freely transferable. At pacific-1 height 237,714,318 there are two allowlists and atlantic-2 has 36, so both networks still end up with none. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@sei-will I've addressed both points from your Slack comments in e964473. On the unbounded work in the upgrade block: On restricted denoms becoming freely transferable: that's intended, and the CHANGELOG entry now says so plainly. For the record, at pacific-1 height 237,714,318 there are two allowlists:
atlantic-2 has 36, all on test denoms. |
4 participants
Tokenfactory is still a fully live module on main. Anyone can create, mint, burn and re-admin
factory/denoms, CosmWasm contracts reach it through thecreate_denom,mint_tokens,burn_tokens,change_adminandset_metadatacustom messages and thetokenfactoryquery route, and both bank keepers enforce its per-denom transfer allowlists. Only its unused governance route is gone so far (#4431). This removes the module outright behind thev6.8upgrade, the same way #4319 removed oracle.x/tokenfactoryandproto/tokenfactoryare deleted and the module is unwired fromapp.New.retiredtokenfactory.ModuleNamejoinsv68DeletedStores, so the store loader deletes thetokenfactorystore at the upgrade height and the handler drops its version-map entry. The handler also deletes the module's parameter subspace from the params store withpruneTokenfactoryParams(on v6.7 that istokenfactory/allowlistmaxsize), so none of its state outlives it. The module account joinsretiredModuleAccountsso its address stays blocked from receiving funds. The six Msg types move to a decode-onlyapp/retiredtokenfactorypackage with the same type URLs and amino names, so historical blocks still decode for tx queries anddebug_trace*. TheirValidateBasicreturnsErrDeprecated(codespacetokenfactory, code 26, because codes 2 to 25 keep their historical meanings), so a new tokenfactory transaction is refused at CheckTx without paying a fee, andretiredtokenfactory_test.gopins their amino sign bytes against bytes captured from the removed module.The
factory/transfer allowlist (IsInDenomAllowListand its setters) goes from bothsei-cosmos/x/bankandgiga/deps/xbank, andpruneDenomAllowListsdeletes the stored entries in the v6.8 handler. Pacific-1 holds only two: a test denom restricted to its own creator and an empty list, so lifting the check changes nothing for a real token. TheAllowListproto type stays in the sei-cosmosbank.protobecause the retiredMsgCreateDenomandMsgUpdateDenomreference it, while giga's unused generated copy of it is removed. Giga's embeddedbank.protodescriptor is left as is: it registers under the same file name as the sei-cosmos one, so it has to keep describingAllowListwhichever registration wins. Contracts that send tokenfactory messages or queries now get the existing "Unknown Sei Wasm Message" and "Unknown Sei Query Route" errors.This is app-hash breaking at the v6.8 height, and unlike oracle there was no deprecation phase: from that height anything still creating, minting or burning
factory/denoms stops working, while existing balances stay in bank, remain transferable, and have a frozen supply. The part to scrutinise isevmrpc/tests/regression_test.go: two pacific-1 fixtures (0x29b9…,0x534a…) executed a CosmWasm contract through the wasmd precompile that emitted tokenfactory messages, so they now trace as reverted with different gas, and they move the way #4230's pointer-creation fixtures did. A third fixture,0xc2da…, calls the same contract but has traced as a revert since #4202, so its gas is unchanged and it now asserts the revert. More generally, tracing a pre-v6.8 block no longer reproduces mainnet where a tokenfactory transaction changed state that a later EVM transaction in the same block read, the limitation the oracle and vesting retirements already accepted. Fixtures that created tokenfactory denoms now use genesisuatom, the authz generic-grant test grantsMsgSendinstead, and the tokenfactory YAML suite, which CI never ran, is deleted. Validated with the v6.8-tagged and untaggedapptests, the bank, giga, wasmbinding, evmrpc,x/, precompile, occ and testutil packages, golangci-lint,make fmtcheck, andmake upgrade-test-offlinefromrelease/v6.7, whose fixture snapshots the params store so a v6.7-written database shows the parameter prune touches nothing else; the cross-version and docker-cluster suites have not run yet.