deprecate vesting module while preserving all store/state#3714
Conversation
|
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 60.14% 59.16% -0.98%
==========================================
Files 2303 2211 -92
Lines 192032 181558 -10474
==========================================
- Hits 115490 107426 -8064
+ Misses 66231 64672 -1559
+ Partials 10311 9460 -851
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The gate is replay-safe: on Wiring: vesting Tests cover deprecated rejection, post-upgrade rejection on Reviewed by Cursor Bugbot for commit c5eff68. 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.
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.
Superseded: latest AI review found no blocking issues.
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.
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>
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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c370ae4 to
c5eff68
Compare
There was a problem hiding this comment.
Deprecates the vesting module via a height-gated, replay-safe consensus rejection while preserving all store/state and existing vesting accounts. The implementation is correct, thorough, and well-tested; only minor operational/process notes remain.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) produced no output; Codex reported no material issues. This synthesis is based on my own analysis plus Codex's empty finding set.
- Release-process coupling:
DeprecationUpgradeName = "v6.7"must match the plan name of the release that actually ships this change. This is well-documented in the PR body and inline comments, but if the change slips to a later release the constant must be bumped or the gate will activate at the wrong height on chains with history. Per repo guideline §1 the not-yet-registered tag itself is expected and not a defect. - The
chainsWithVestingHistoryallowlist (pacific-1,atlantic-2,arctic-1) is the sole determinant of replay-preserving behavior; any other chain with pre-deprecation vesting history that is not listed would reject from genesis and diverge on replay. This matches Sei's public-chain deployment model, but is worth keeping in mind if additional networks are ever migrated.
Superseded: latest AI review found no blocking issues.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found in this pass, but given this PR is labeled app-hash-breaking and rewires consensus-level vesting logic (height-gated rejection, msg-server/module wiring across the main app, sei-wasmd, and sei-ibc-go simapps), it's worth a human's sign-off rather than an automated approval.
What was reviewed: the creationDeprecated height-gate and its interaction with IsUpgradeActiveAtHeight/gas-free lookup in msg_server.go; the module/handler wiring changes across app.go, sei-wasmd/app/app.go, and sei-ibc-go/testing/simapp/app.go; CLI/genesis deprecation notices; and the new/updated test coverage in handler_test.go and the vesting CLI integration suite.
Also checked: the concern that state-export + re-genesis under the same chain-id could silently revert the gate (since x/upgrade's ExportGenesis is a no-op and doesn't persist done-heights) — this is pre-existing, systemic behavior of the upgrade module shared by every other upgrade-gated feature in the codebase, not something newly introduced here, and Sei's re-genesis events change chain-id in practice, so it isn't a new bug from this diff.
Extended reasoning...
Overview
This PR deprecates the vesting module's account-creation path while preserving existing vesting state and codecs. Core changes are the height-gated rejection in sei-cosmos/x/auth/vesting/msg_server.go (using UpgradeKeeper.IsUpgradeActiveAtHeight with a gas-free context to keep historical gas usage byte-identical), the corresponding wiring of UpgradeKeeper through AppModule/NewHandler/NewMsgServerImpl in the main app, sei-wasmd, and the sei-ibc-go test simapp, plus CLI/genesis-tooling deprecation notices and test coverage updates.
Security risks
No new attack surface (auth, crypto, permissions) is introduced. The main risk category here is consensus/app-hash correctness rather than a traditional security vulnerability: an incorrect gate (wrong upgrade name, wrong height comparison, wrong chain-id allowlist) would cause a chain halt or app-hash divergence across validators. The diff has already been through two rounds of blocker-level review (unconditional-rejection bug, then a stale upgrade-name mismatch), both fixed in follow-up commits, and the final state (DeprecationUpgradeName = \"v6.7\", not yet in app/tags) matches this repo's normal release-cut process per REVIEW.md.
Level of scrutiny
This warrants a high level of scrutiny: it's labeled app-hash-breaking, changes behavior of a consensus message handler on multiple live chains (pacific-1, atlantic-2, arctic-1), and touches shared app wiring in three separate app.go files. This is exactly the category of change (large-scale design decision + critical code path) that should get a human sign-off rather than automated approval, independent of whether bugs were found.
Other factors
The PR has solid test coverage for all three gate states (fresh-chain rejection, pre-upgrade historical preservation, post-upgrade rejection) plus a dedicated test for existing vesting accounts remaining functional. Prior automated reviewers (Cursor Bugbot, seidroid) raised several concerns that were addressed via follow-up commits, and I did not find any unresolved bugs in the current diff.
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