Repository navigation
Remove the vesting module - #4328
Conversation
Delete x/auth/vesting, its protos, the vesting CLI and the add-genesis-account vesting flags, and add a v6.8 upgrade whose handler runs the new auth 3 to 4 migration and deletes the vesting module version. The migration rewrites every account stored under a vesting type as the BaseAccount it embeds, so balances a schedule still locked become spendable. Bank's LockedCoins now returns no coins and reads the account only when re-tracing a block from before v6.8, so debug_trace of historical blocks keeps the gas those blocks used while live execution skips the read. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
@seidroid review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4328 +/- ##
==========================================
- Coverage 67.65% 66.42% -1.24%
==========================================
Files 2166 2064 -102
Lines 168085 155494 -12591
==========================================
- Hits 113717 103280 -10437
+ Misses 54359 52204 -2155
- Partials 9 10 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
main's v6.8 precompile generation (#4320) appended v6.8 to app/tags and scaffolded the v6.7 to v6.8 boundary tests, which this branch had also written. The resolution keeps main's scaffold and adds the vesting removal on top of it. The orphan test keeps main's retainedStores, which now name the upgrade that removed each module, and lists vesting, which owned no store, separately. The in-process tests keep main's upgrade and un-upgraded halt tests and its pre-upgrade bank send in the cross-version run, and the offline phases keep main's store-set checks next to the account rewrite checks. The only main assertions replaced are those that the upgrade leaves the module version map unchanged, since v6.8 now deletes vesting's entry and moves auth to consensus version 4. Co-authored-by: Cursor <cursoragent@cursor.com>
The live cross-version run pins the whole version map v6.8 leaves behind, and failed because the upgrade also runs gov's 3 to 4 migration from #4000, which release/v6.7 does not carry. Record that move alongside the vesting removal and the auth migration. Co-authored-by: Cursor <cursoragent@cursor.com>
Move MsgCreateVestingAccount into a decode-only app/retiredvesting package with the same type URL and fields, as app/retiredibc and the oracle removal do for their messages, so transactions from before v6.8 still decode. Its ValidateBasic returns the error v6.7's message server returned, codespace vesting and code 2, so a new one is refused at CheckTx and in DeliverTx before any fee is charged, where v6.7 charged the fee first. The vesting account types stay unregistered: after the auth 3 to 4 migration no account in state carries one, and registering them would let a genesis file create vesting accounts nothing enforces. Co-authored-by: Cursor <cursoragent@cursor.com>
Add the pacific-1 MsgCreateVestingAccount transaction 2838170E...0709, included at height 228,422,853, as a fixture. The test checks the bytes against the transaction hash, decodes them through both app transaction configs, and requires the retired type to carry the fields the chain recorded and to encode the message back to the bytes it included. The hand-encoded fixture stays for the admin field, which that transaction leaves unset. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
PR SummaryHigh Risk Overview On-chain migration: auth consensus version 3→4 ( Compatibility: Tests expand v6.8 coverage (rewrite, spend formerly locked balance, genesis export, cross-version) and offline upgrade fixtures for vesting account migration. Reviewed by Cursor Bugbot for commit 1ad0a6e. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR removes the vesting module cleanly. The v6.8 handler follows the v6.7 pattern, and the auth 3→4 migration parses the raw account bytes carefully, refuses anything it cannot convert, and has tests. There are no blockers; one note remains on how precise the gas gate is when re-tracing blocks from before v6.8.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Consider an app-level test that re-traces a block from before v6.8 through the real RPC context provider. It would check that
ClosestUpgradeNameresolves below v6.8 there and that the gas matches what the block originally used. The current tests set the upgrade name by hand, so they don't cover the choice of store/height that the gate depends on. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Add an evmrpc test that re-traces blocks around v6.7 and v6.8 upgrades, recorded at their heights, through StateAtBlock and the app's RPCContextProvider rather than an upgrade name set by hand. The provider names the first upgrade applied at or after a block's parent. The test requires bank's locked-coins lookup to read the account, as every binary before v6.8 did, through the first block after the v6.7 upgrade, and to skip the read from the v6.8 upgrade block on. It also pins the gap in between: v6.7 blocks after the first two are named v6.8 and re-traced without the read their execution made. RetracesLockedCoinsLookup now documents that gap and that v6.7 freeze nodes serve those traces, and neither LockedCoins claims any longer that its trace consumes the gas the block did. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
This removes the vesting module cleanly. The new commit covers both earlier requests: RetracesLockedCoinsLookup's godoc now states that once v6.8 is applied, most v6.7 blocks re-trace without the account read and that v6.7 freeze nodes serve those traces, and an app-level test re-traces blocks through the real RPCContextProvider to pin that behavior. No new issues found.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
v6.7 rejects
MsgCreateVestingAccountonce its upgrade has run, but kept the module wired in so existing vesting accounts still decode. The only ones left are test accounts: three on pacific-1 holding 2.1 SEI locked until the year 3000 (a full scan of the account store on 24 September found no others, and the scan is repeated before the upgrade), and a few dozen already fully vested ones on atlantic-2. Keeping the module means bank reads and decodes an account on every debit, delegation and EVM balance lookup just to learn that nothing is locked.This deletes
x/auth/vesting, its protos,seid tx vestingand theadd-genesis-account --vesting-*flags, and adds av6.8upgrade. Its handler runs the new auth 3 to 4 migration,Migrator.Migrate3to4, then deletes thevestingmodule version. The migration scans the account store once and rewrites every account stored under a vesting type as theBaseAccountit embeds, keeping address, public key, account number and sequence; balances are untouched, so locked coins become spendable. It parses the raw bytes because the vesting account types are gone, and it returns an error, halting the upgrade, rather than leave behind an account the new binary cannot decode. A full scan beats a hardcoded address list because atlantic-2's tx index does not cover its whole history; through the real store stack it costs about 235 ns per account, roughly 25 s of CPU for pacific-1's 102M accounts plus disk reads.MsgCreateVestingAccountitself moves to a decode-onlyapp/retiredvestingpackage with its type URL and fields, asapp/retiredibcand the oracle removal in #4319 do for their types, so transactions from before v6.8 still decode.Bank's
LockedCoinsstays as the one place that used to read the account. It now returns no coins and performs the read only whenRetracesLockedCoinsLookupreports a re-trace of a block from before v6.8, the same pattern as distribution'sReadOnlyRewardsUpgrade, and delegation keeps its account lookup behind the same gate. Without this, the evmrpc mainnet regression replays fail: traces of old precompile calls change gas, and one historical out-of-gas transaction starts succeeding. The gate is only as precise as theClosestUpgradeNamethe RPC context carries, andGetClosestUpgradereturns the next upgrade rather than the one in effect, so once v6.8 is applied most v6.7-era blocks re-trace without the read; v6.7 freeze nodes serve those traces. Live execution skips the read, so transactions and precompile calls that move coins use less gas (1,405 per bank debit in the updated wasm tests), and delegating or undelegating no longer requires the delegator to have an account.This is state-machine breaking and ships behind
v6.8, which also runs gov's 3 to 4 migration from #4000, so the upgrade moves auth and gov to version 4 and dropsvestingfrom the version map. After the upgradeMsgCreateVestingAccountstill decodes, but itsValidateBasicreturns v6.7's deprecation error (codespacevesting, code 2), so it is refused at CheckTx and in DeliverTx before any fee, where v6.7 charged the fee first. The vesting account types are deliberately not retired: the migration leaves none in state, and registering them would let a genesis file create vesting accounts nothing enforces, so v6.8 nodes cannot decode vesting accounts from before the upgrade and v6.7 freeze nodes serve those historical queries.x/auth/keeper/legacy_vesting.goand the gate inx/bank/keeper/view.godeserve the closest look. Validated with the new v6.8 in-process tests, the offline source, target and reopen phases run againstrelease/v6.7,make upgrade-test-vet, the affected package suites, and the live v6.7 to v6.8 cross-version run in CI's Minor release boundary job.