deprecate vesting module while preserving all store/state#3714
Conversation
Gate the vesting module at the consensus level: MsgCreateVestingAccount (the module's only message) is now rejected by the msg server with a registered ErrVestingDeprecated (codespace "vesting", code 2), covering both the gRPC msg service and the legacy Route() path. All state support is preserved: codec/interface/amino registrations, the vesting account types, and bank/xbank locked-coin logic are untouched, so existing vesting accounts keep decoding and vesting, and historical transactions remain decodable. The module stays wired into all apps for exactly that reason. With the handlers gated, the keeper plumbing became dead code and is removed: msgServer is now an empty struct, NewMsgServerImpl/NewHandler/ NewAppModule take no keepers, and the unused BankKeeper expected-keeper interface, AttributeValueCategory constant, and vesting metrics are deleted. Client tooling is deprecated but functional: the CLI tx command carries a cobra deprecation notice, and the add-genesis-account vesting flags are marked deprecated for legacy genesis tooling. Also repairs the norace-gated vesting CLI integration suite, which had rotted (stale network.DefaultConfig signature, fees below the 2000usei chain minimum); it now runs green end-to-end and asserts the deprecation error code on-chain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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 #3714 +/- ##
==========================================
- Coverage 59.30% 58.35% -0.96%
==========================================
Files 2274 2188 -86
Lines 188706 179322 -9384
==========================================
- Hits 111917 104642 -7275
+ Misses 66716 65381 -1335
+ Partials 10073 9299 -774
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview The gate is replay-safe: on Wiring passes Reviewed by Cursor Bugbot for commit c370ae4. 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.
A clean, well-tested deprecation of the vesting module that preserves all state/codec registrations; the code changes are correct and self-consistent. The one blocking concern is that the message rejection is unconditional rather than gated behind the upgrade height, which risks breaking from-genesis replay of any historical vesting-account creation.
Findings: 2 blocking | 3 non-blocking | 1 posted inline
Blockers
- Consensus/replay gating:
MsgCreateVestingAccountis now rejected unconditionally (msg server + legacyRoute()handler). This is an app-hash-breaking change (per the PR label). If any historical block on mainnet/testnet contains a successfulMsgCreateVestingAccount, a node replaying from genesis with this binary will reject that message, produce different state at that height, and fail app-hash verification (unable to sync from genesis). Standard practice for a live chain is to gate the new behavior behind the coordinated upgrade block height so historical blocks execute with the old logic. Please confirm either (a) the rejection is/should be height-gated, or (b) no historical vesting-creation txs exist and from-genesis replay is not a supported path — and document that reasoning. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Both second-opinion passes produced no findings: Codex reported "No material findings" (noting it could not run the vesting tests due to sandboxed network access), and the Cursor review file was empty.
REVIEW_GUIDELINES.mdwas also empty, so no repo-specific standards were applied. ErrVestingDeprecatedis registered with code 2 in codespacevesting(errors.go). Confirm code 2 was chosen deliberately (code 1 is skipped); ensure no other registration in thevestingcodespace collides, sincesdkerrors.Registerpanics on duplicates.- The CLI test (suite.go) now skips output up to the first
{to bypass cobra's deprecation notice before JSON unmarshalling. This is a bit fragile (assumes the response is JSON and contains no leading{in the notice); acceptable for a test but worth a brief comment noting the assumption.
| // types.ErrVestingDeprecated. Existing vesting accounts remain in state and | ||
| // continue to vest according to their schedules; only the creation of new | ||
| // vesting accounts is disabled. | ||
| func (s msgServer) CreateVestingAccount(context.Context, *types.MsgCreateVestingAccount) (*types.MsgCreateVestingAccountResponse, error) { |
There was a problem hiding this comment.
[blocker] This rejection is unconditional — it fires for every block height, not just after the deprecation upgrade. Because this change is app-hash-breaking, replaying a historical block that previously contained a successful MsgCreateVestingAccount will now return ErrVestingDeprecated, yielding different state and an app-hash mismatch (breaking from-genesis sync). Consider gating on the upgrade block height so historical execution is unchanged, or confirm no historical vesting-creation txs exist and document why unconditional rejection is safe.
There was a problem hiding this comment.
Good catch — fixed in 75553c6. The rejection is now height-gated so historical execution is byte-identical:
- On chains with pre-deprecation history (
pacific-1,atlantic-2,arctic-1), the original creation logic is preserved below the height at which the deprecation release's upgrade executes. The gate usesUpgradeKeeper.IsUpgradeActiveAtHeight(ctx, DeprecationUpgradeName, height)with an infinite gas meter for the done-height lookup (same pattern asx/evm/ante/fee.go), so gas consumption of historical transactions is also unchanged — replay reproduces identical state, gas, and app hashes. From the upgrade height onward, the message is rejected withErrVestingDeprecated. - On every other chain (fresh networks, localnets, tests), the deprecation is active from genesis.
This also fixes a second, related issue with the unconditional version: a validator adopting the new binary before the scheduled upgrade height would have diverged from the network; with the gate, behavior switches exactly at the upgrade block.
One release-process note: DeprecationUpgradeName is set to "v6.6" (the latest entry in app/tags; v6.6.0-rc1 is the current pre-release, so the upgrade has not executed on any public chain yet). It must match the plan name of the release that actually ships this change — if this lands in a later release instead, the constant needs to be bumped, since GetDoneHeight is an exact-name lookup.
Address review feedback: the unconditional rejection was app-hash-breaking for from-genesis sync, since replaying a historical block containing a successful MsgCreateVestingAccount would produce different state, gas, and tx results. The deprecation is now gated per chain: - pacific-1 / atlantic-2 / arctic-1 (chains with pre-deprecation history): the original creation logic is preserved below the height at which the deprecation release's upgrade (DeprecationUpgradeName, "v6.6") executes, using UpgradeKeeper.IsUpgradeActiveAtHeight with a gas-free lookup so historical gas consumption is unchanged; from the upgrade height onward the message is rejected with ErrVestingDeprecated. - every other chain (fresh networks, tests): rejected from genesis. This also fixes early-binary-adoption divergence: validators running the new binary before the scheduled upgrade height keep consensus with the network. The msg server, handler, and module take back their keeper wiring (plus the upgrade keeper) to support the preserved legacy path; the telemetry- only vesting metrics remain removed. Tests cover all three gate states. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
A clean, well-documented deprecation of the vesting module that preserves all store/state and gates rejection behind a replay-safe, height-gated upgrade check with good test coverage. No blocking issues; two non-blocking robustness/process concerns around the durability of the upgrade-done gate.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- cursor-review.md was empty — the Cursor second-opinion pass produced no output, so only Codex's review was available for merging.
- The whole gate for pacific-1/atlantic-2/arctic-1 depends on a governance upgrade plan named exactly
v6.6executing and setting a done-height.RegisterUpgradeHandlerssources names from the embeddedtagsfile, so if the shipping release's plan name/tag differs fromDeprecationUpgradeName("v6.6"), the deprecation silently never activates on mainnet with no test to catch it. Consider asserting the constant against the release tag or adding a startup log/check. - Test coverage is strong (fresh-chain reject, pacific-1 pre-upgrade original behavior, pacific-1 post-upgrade reject, existing-account still-vests). Minor nit: the CLI test's
bytes.IndexByte(out, '{')heuristic to skip cobra's deprecation notice is fragile if the notice ever contains a brace, though currently safe. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // performed no store reads before its first bank check, so charging gas | ||
| // here would alter gas usage of historical transactions during replay. | ||
| gasFreeCtx := ctx.WithGasMeter(sdk.NewInfiniteGasMeter(1, 1)) | ||
| return s.upgradeKeeper.IsUpgradeActiveAtHeight(gasFreeCtx, DeprecationUpgradeName, ctx.BlockHeight()) |
There was a problem hiding this comment.
[suggestion] Confirming Codex's finding: this gate depends on UpgradeKeeper's done-height for v6.6, but the upgrade module's ExportGenesis is empty (sei-cosmos/x/upgrade/module.go:128-131) and does not preserve done heights. After a post-v6.6 state export + re-genesis that keeps the same chain-id (e.g. pacific-1), IsUpgradeActiveAtHeight returns false permanently and new vesting accounts become creatable again — silently un-deprecating the module. Chain-id typically changes on such restarts, but if it doesn't the gate reverts. Consider a more durable signal (e.g. also gate on a hardcoded block height floor, or persist a dedicated deprecation flag) so the deprecation survives re-genesis.
| // module's deprecation on chains with pre-deprecation history. It must match | ||
| // the plan name of the release that ships the deprecation; if this change | ||
| // slips to a later release, bump this constant to that release's name. | ||
| const DeprecationUpgradeName = "v6.6" |
There was a problem hiding this comment.
[suggestion] This constant must exactly match the plan name of the release that ships this change (and the entry added to the embedded tags file). If they diverge, the done-height for this name never gets set on the history chains and the deprecation becomes a permanent no-op on mainnet/testnet/devnet with no test failure. Worth adding an automated safeguard tying this to the release tag rather than relying on the PR-description reminder.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cleanly-scoped deprecation of the vesting module: the msg-server gate covers the only on-chain entry point (CreateVestingAccount), state/codec registrations are preserved for replay, and test coverage is solid. The main open risk is that DeprecationUpgradeName is set to "v6.7" while the app/tags registry currently ends at "v6.6", so the deprecation gate stays inert (fail-open) on pacific-1/atlantic-2/arctic-1 until a matching upgrade is registered and executed.
Findings: 1 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Release coupling: the deprecation only takes effect on historical chains once an upgrade named
DeprecationUpgradeNameis registered (added toapp/tags) and executes on-chain. This PR does not addv6.7toapp/tags(latest entry isv6.6), so as-is the gate never activates on pacific-1/atlantic-2/arctic-1. Ensure the constant matches the actual shipping release plan and thatapp/tagsis updated in the same release; otherwise the module remains fully un-deprecated on mainnet/testnet/devnet. (Also raised by Codex.) - Tests reuse
DeprecationUpgradeNameinSetDone(...), so a wrong plan-name value would still pass all tests — the gate-name correctness is not independently asserted. Consider an assertion tying the constant to a registered upgrade tag, or a test that the gate is off before and on after the real upgrade name. - Cursor's second-opinion review (
cursor-review.md) produced no output; only Codex's pass contributed findings, which centered on the sameDeprecationUpgradeNameconcern. suite.goskips to the first{in the captured buffer to strip cobra's deprecation notice before JSON-unmarshalling. This is fine for the current notice text (no braces), but is fragile if the deprecation message ever contains{; a more targeted split (e.g. on the known notice prefix/newline) would be sturdier.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // module's deprecation on chains with pre-deprecation history. It must match | ||
| // the plan name of the release that ships the deprecation; if this change | ||
| // slips to a later release, bump this constant to that release's name. | ||
| const DeprecationUpgradeName = "v6.7" |
There was a problem hiding this comment.
[blocker] DeprecationUpgradeName = "v6.7" is ahead of the currently-registered upgrade set — app/tags ends at v6.6 and this PR doesn't add v6.7. Since IsUpgradeActiveAtHeight returns false when the done-height is 0, creationDeprecated is fail-open: on pacific-1/atlantic-2/arctic-1 vesting-account creation stays enabled until a v6.7 upgrade is both registered in app/tags and executed. Please confirm this ships in the v6.7 release and that app/tags gains v6.7 in that same release; if it actually ships in v6.6, this constant must be "v6.6". Getting this wrong silently no-ops the deprecation on production chains.
| suite.Require().ErrorIs(err, types.ErrVestingDeprecated) | ||
| suite.Require().Nil(res) | ||
|
|
||
| // no account is created and no funds move |
There was a problem hiding this comment.
[nit] This test uses DeprecationUpgradeName for SetDone, so it verifies the gate mechanism but not that the constant names the real shipping upgrade — a wrong plan name (e.g. v6.7 vs v6.6) would still make this test pass while the gate never fires on mainnet. Consider asserting the constant against the registered upgrade tag list.
3 participants
Summary
Deprecates the vesting module and vesting account creation without removing any store/state. Existing vesting accounts remain fully supported — they keep decoding, vesting, and locking spendable coins — but no new vesting accounts can be created once the deprecation gate is active.
Consensus-level gate (replay-safe)
MsgCreateVestingAccount(the module's only message) is rejected by the msg server with a new registered errorErrVestingDeprecated(codespacevesting, code 2), covering both the gRPC msg service and the legacyRoute()handler path.pacific-1,atlantic-2,arctic-1), the original creation logic is preserved below the height at which the deprecation release's upgrade (DeprecationUpgradeName = "v6.7") executes, viaUpgradeKeeper.IsUpgradeActiveAtHeightwith a gas-free done-height lookup — so replaying history produces identical state, gas, and app hashes, and validators adopting the binary early stay in consensus. On all other chains (fresh networks, tests), the deprecation is active from genesis.DeprecationUpgradeNamemust match the plan name of the release that ships this change. If this lands in a release later than v6.7, bump the constant.State stays fully supported
app.go(and the sei-wasmd / sei-ibc-go simapps) — required to decode existing vesting accounts in the auth store and historical transactions.Dead code removal
metrics.go) are removed; theAttributeValueCategoryconstant andBankKeeperexpected-keeper interface remain in use by the replay-safe legacy path, which keeps the original keeper wiring.Client tooling (deprecated, not removed)
seid tx vesting create-vesting-accountcarries a cobra deprecation notice (printed to stderr in production).seid add-genesis-account --vesting-*flags are marked deprecated but still work for legacy genesis tooling.EVM side
Audited for a vesting precompile: none exists. The precompile registry (bank, wasmd, json, addr, staking, gov, distribution, oracle, ibc, pointer, pointerview, p256, solo) and giga's executor precompiles have no vesting surface, and nothing consumes the vesting
Adminfield. The msg-server gate therefore covers every on-chain entry point.Test coverage
handler_test.gocovers all three gate states: fresh chains reject from genesis with no state mutation;pacific-1below the upgrade height preserves the original behavior (full original test matrix, including admin cases);pacific-1with the upgrade executed rejects. Plus a test documenting that vesting accounts already in state keep working.norace-gated vesting CLI integration suite, which had rotted (stalenetwork.DefaultConfigsignature; fees below the 2000usei chain minimum). It runs green end-to-end and asserts the deprecation code on-chain.Test plan
go buildon the main app, sei-wasmd, sei-ibc-go simapp,cmd, andx/evmgo test -race ./sei-cosmos/x/auth/vesting/...andgo test -race -run TestABCI ./x/evm/go test -tags norace -run TestIntegrationTestSuite ./sei-cosmos/x/auth/vesting/client/testutil/(broadcasts the tx, receives the deprecation error code)go vet(incl.-tags norace),gofmt -s -l,goimports -lall clean🤖 Generated with Claude Code