refactor(vaults): move new configuration values to state#53
Conversation
WalkthroughDeprecates two module config fields, removes BeginBlocker support, migrates the Vaults Season Two yield collector from in-memory/config to persisted state and genesis, updates keeper constructor and call sites, adjusts message server and genesis handling, updates protos and generated code, and tweaks init/local scripts and app config. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppModule
participant K as Keeper
participant State as Store
App->>K: InitGenesis(genesis)
K->>State: Set VaultsSeasonTwoYieldCollector([]byte)
App->>K: ExportGenesis()
K->>State: Get VaultsSeasonTwoYieldCollector() -> []byte
K-->>App: return address (from bytes)
sequenceDiagram
participant Msg as MsgServer
participant K as Keeper
participant Bank as BankKeeper
participant State as Store
Msg->>K: handleVaultsYieldSeasonTwo(ctx)
K->>State: Get VaultsSeasonTwoYieldCollector() -> []byte
K-->>Msg: sdk.AccAddress
Msg->>Bank: SendCoins(..., collector)
Bank-->>Msg: result
sequenceDiagram
participant Msg as MsgServer
participant K as Keeper
Msg->>K: Lock/Unlock(ctx, FLEXIBLE)
K-->>Msg: IsVaultsSeasonOneEnded(ctx)?
alt season ended
Msg-->>Msg: return season-ended error
else not ended
Msg-->>Msg: proceed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
keeper/msg_server.go (1)
471-489: Guard against unset Season Two yield collector before transferring funds.If the collector address was not initialized (e.g., invalid/empty in genesis), SendCoins will fail with a less-informative error. Add a clear pre-check and return a descriptive error.
Apply this diff to harden the transfer:
func (k *Keeper) handleVaultsYieldSeasonTwo(ctx context.Context) error { @@ // Ensure that there is a valid amount of yield to send. if !yield.IsPositive() { return nil } // Send the Staked vault yield to the Collector address. - err = k.bank.SendCoins(ctx, vaults.StakedVaultAddress, k.GetVaultsSeasonTwoYieldCollector(ctx), sdk.NewCoins(sdk.NewCoin(k.denom, yield))) + collector := k.GetVaultsSeasonTwoYieldCollector(ctx) + if len(collector) == 0 { + return fmt.Errorf("vaults season two yield collector address is not set") + } + err = k.bank.SendCoins(ctx, vaults.StakedVaultAddress, collector, sdk.NewCoins(sdk.NewCoin(k.denom, yield))) if err != nil { return err } return nil }And add this import outside the selected range:
import "fmt"
🧹 Nitpick comments (5)
proto/noble/dollar/vaults/v1/genesis.proto (1)
37-38: Annotate the address as a Cosmos address scalar for better tooling/validationMarking the field as a Cosmos AddressString helps downstream tooling and linting, and documents intent. Optional but recommended.
Apply this diff:
- // season_two_yield_collector defines the yield collector during Vaults Season Two. - string season_two_yield_collector = 8; + // season_two_yield_collector defines the yield collector during Vaults Season Two. + string season_two_yield_collector = 8 [ + (cosmos_proto.scalar) = "cosmos.AddressString" + ];Follow-up: ensure genesis validation rejects an empty or invalid bech32 address (can be enforced in InitGenesis or a ValidateGenesis routine).
local.sh (1)
29-30: Reduce repeated I/O: set both vaults fields in a single jq mutationFewer writes and a smaller window for partial updates.
Apply this diff:
- touch $TEMP && jq '.app_state.dollar.vaults.season_one_ended = true' .dollar/config/genesis.json > $TEMP && mv $TEMP .dollar/config/genesis.json - touch $TEMP && jq '.app_state.dollar.vaults.season_two_yield_collector = "noble1zw7vatnx0vla7gzxucgypz0kfr6965akpvzw69"' .dollar/config/genesis.json > $TEMP && mv $TEMP .dollar/config/genesis.json + touch $TEMP && jq '.app_state.dollar.vaults += {season_one_ended: true, season_two_yield_collector: "noble1zw7vatnx0vla7gzxucgypz0kfr6965akpvzw69"}' .dollar/config/genesis.json > $TEMP && mv $TEMP .dollar/config/genesis.jsonAdditionally (outside this hunk), consider adding
set -euo pipefailat the top of the script to fail fast on errors.keeper/msg_server.go (1)
471-489: Optional: emit an event when forwarding staked yield to the collector.For observability and auditing, consider emitting an event with amount and collector address after a successful SendCoins.
proto/noble/dollar/module/v1/module.proto (1)
25-29: Good deprecation of module-level vault fields; add guidance to the replacement.Consider clarifying comments to point users to the new source of truth (genesis.vaults.v1.GenesisState.season_two_yield_collector) to avoid confusion.
Suggested comment tweak:
- // vaults_season_one_end_timestamp defines the Unix timestamp in seconds when Vaults Season One ends. + // DEPRECATED: use the persisted state/genesis field in vaults.v1.GenesisState (season_one_ended). int64 vaults_season_one_end_timestamp = 5 [deprecated = true]; - // vaults_season_two_yield_collector defines the address to collect Staked vault yield during Season Two. + // DEPRECATED: use vaults.v1.GenesisState.season_two_yield_collector and the state-backed getter in Keeper. string vaults_season_two_yield_collector = 6 [deprecated = true];genesis.go (1)
38-180: Add a unit test to cover invalid SeasonTwoYieldCollector in genesis.Given the new state-backed field, add tests to assert:
- panic on invalid bech32/format (after applying the error-handling fix).
- successful round-trip: InitGenesis -> ExportGenesis preserves the collector.
I can draft table-driven tests for InitGenesis/ExportGenesis covering valid/invalid collectors. Want me to open an issue and provide the test file?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
types/vaults/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
Makefile(1 hunks)api/module/v1/module.pulsar.go(4 hunks)api/vaults/v1/genesis.pulsar.go(16 hunks)genesis.go(3 hunks)keeper/abci.go(0 hunks)keeper/keeper.go(5 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_vaults.go(2 hunks)keeper/msg_server_vaults_test.go(0 hunks)local.sh(1 hunks)module.go(0 hunks)proto/noble/dollar/module/v1/module.proto(1 hunks)proto/noble/dollar/vaults/v1/genesis.proto(1 hunks)simapp/app.yaml(1 hunks)types/vaults/keys.go(1 hunks)utils/mocks/dollar.go(0 hunks)
💤 Files with no reviewable changes (4)
- keeper/abci.go
- keeper/msg_server_vaults_test.go
- utils/mocks/dollar.go
- module.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
keeper/msg_server_vaults.go (1)
types/vaults/vaults.pb.go (1)
FLEXIBLE(40-40)
keeper/msg_server.go (1)
types/vaults/keys.go (1)
StakedVaultAddress(36-36)
api/vaults/v1/genesis.pulsar.go (1)
types/vaults/genesis.pb.go (3)
GenesisState(30-45)GenesisState(49-49)GenesisState(50-52)
keeper/keeper.go (2)
keeper/state_vaults.go (2)
VaultsPositionsIndexes(33-36)NewVaultsPositionsIndexes(45-64)types/vaults/keys.go (7)
PausedKey(43-43)SeasonOneEndedKey(44-44)SeasonTwoYieldCollectorKey(45-45)PositionPrefix(47-47)TotalFlexiblePrincipalKey(46-46)RewardPrefix(48-48)StatsKey(49-49)
🔇 Additional comments (17)
types/vaults/keys.go (1)
43-49: LGTM: New state key is consistent with existing keyspace layoutSeasonTwoYieldCollectorKey aligns with the vaults key naming scheme and avoids collisions with existing prefixes.
simapp/app.yaml (1)
6-6: BeginBlocker removal confirmed
NoBeginBlockerfunctions or registrations remain in the codebase; only a historical note exists in CHANGELOG.md.api/vaults/v1/genesis.pulsar.go (4)
120-141: Generated descriptor wiring for SeasonTwoYieldCollector looks correct.fd initialized and bound via init(); field name matches proto; no inconsistencies spotted.
244-249: Fast reflection Range includes SeasonTwoYieldCollector only when non-empty — LGTM.Conditional emission matches proto3 semantics for optional strings.
560-564: Size/Marshal/Unmarshal paths for field 8 are consistent with wire format.
- Size accounts for length-delimited string with 1-byte tag (0x42).
- Marshal emits 0x42 (field=8, wire=2) and length-prefixed string.
- Unmarshal handles tag 8, wire type 2, and decodes string safely.
No issues found.
Also applies to: 593-599, 892-924
990-992: Struct field and accessor for SeasonTwoYieldCollector are aligned with the proto.Getter returns empty string for nil receiver. Good.
Also applies to: 1056-1061
keeper/msg_server.go (1)
471-489: AllvaultsSeasonTwoYieldCollectorreferences have been migrated to the state-backed getter
I ran a global search and confirmed:
- No occurrences of
k.vaultsSeasonTwoYieldCollectorremain.- All uses are via
GetVaultsSeasonTwoYieldCollector(ctx)or the KV‐store setter ingenesis.go.- There are no
BeginBlockerremnants in the code.LGTM.
keeper/msg_server_vaults.go (2)
49-51: Blocking FLEXIBLE locks after Season One end is correct.Policy aligns with Season One end semantics. Error type and message are consistent with existing patterns.
156-159: Blocking FLEXIBLE unlocks after Season One end is correct.Matches locking behavior and avoids inconsistent state transitions post Season One.
genesis.go (1)
201-221: Exporting SeasonTwoYieldCollector via getter is correct and context-aware.Getter-based export ensures the value mirrors persisted state, matching the PR objective to migrate from config to state.
api/module/v1/module.pulsar.go (3)
819-854: Raw descriptor updates look consistent with deprecations.The updated bytes include the deprecated flag markers for the two fields. Make sure this file is purely generated and kept in sync with proto sources (avoid manual edits).
794-801: No remaining call sites forGetVaultsSeasonOneEndTimestamp()The search confirms that this deprecated getter is only defined at
api/module/v1/module.pulsar.go:795and is never called elsewhere. You can safely remove this method.
802-809: No remaining deprecated Module accessor callsVerification shows that all usages now invoke the Keeper-backed method:
- genesis.go:202 calls
k.GetVaultsSeasonTwoYieldCollector(ctx).String()- keeper/msg_server.go:483 calls
k.GetVaultsSeasonTwoYieldCollector(ctx)No direct calls to
(*Module).GetVaultsSeasonTwoYieldCollector()remain.keeper/keeper.go (4)
51-54: No issues with retained config fields.Keeping denom/authority/min lock/unlock as in-memory config is consistent with the PR scope. No concerns here.
125-129: Keeper ctor params trimmed (LGTM).Constructor no longer takes season-one/two config, aligning with state persistence. Call sites should be updated accordingly (as noted in PR).
158-165: Initializing SeasonTwoYieldCollector as bytes (LGTM).Using collections.BytesValue is fine; conversion to sdk.AccAddress can happen at the accessor. Ensure genesis/init fills this item as expected (PR summary indicates it does).
84-91: VaultsPaused storage type is correctThe audit confirms that VaultsPaused is intentionally persisted as an int32-backed enum (
vaults.PausedType), with all call sites casting to/from int32 consistently:
- genesis.go line 160:
k.VaultsPaused.Set(ctx, int32(genesis.Vaults.Paused))- keeper/msg_server_vaults.go line 354:
k.VaultsPaused.Set(ctx, int32(msg.Paused))- keeper/state_vaults.go line 79:
value, _ := k.VaultsPaused.Get(ctx)→ returned asvaults.PausedTypeNo changes are required.
jtieri
left a comment
jtieri
left a comment
There was a problem hiding this comment.
I had a couple questions around errors that are being ignored but otherwise the code changes look good.
g-luca
left a comment
g-luca
left a comment
There was a problem hiding this comment.
Agree with @jtieri comments, but besides of these changes everything else looks good to me! The only comment that I have here is that with this approach we won't have anymore an easy way to check the collector/ended values as there are no queries. (especially if the collector will stay alive even after the managed vaults update)
Is this an issue as these values are only set once (through the v11 upgrade handler)? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/utils.go (2)
110-110: Avoid hard-coding the collector address; factor into a const to prevent driftMinor: extracting the bech32 address to a single const reduces duplication and makes it easier to coordinate with local.sh/simapp config.
Apply this diff within the selected range:
- cosmos.NewGenesisKV("app_state.dollar.vaults.season_two_yield_collector", "noble1zw7vatnx0vla7gzxucgypz0kfr6965akpvzw69"), + cosmos.NewGenesisKV("app_state.dollar.vaults.season_two_yield_collector", seasonTwoCollectorAddr),Add this declaration once near the top of the file (outside the selected range), e.g., after imports:
const seasonTwoCollectorAddr = "noble1zw7vatnx0vla7gzxucgypz0kfr6965akpvzw69"
109-110: Optional: add a post-boot assertion path for these valuesGiven the comment thread about lacking an easy way to check collector/ended, consider adding a thin query surface (e.g., x/vaults Query service exposing SeasonOneEnded and SeasonTwoYieldCollector) and assert them here after chain boot. I can help wire a minimal query and the corresponding E2E assertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
e2e/utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
e2e/utils.go (2)
109-110: LGTM: genesis keys align with the new state-backed vaults designSetting app_state.dollar.vaults.season_one_ended and app_state.dollar.vaults.season_two_yield_collector at genesis matches the migration away from module config and should unblock E2E scenarios that require Season Two.
109-110: Verified JSON field names match vaults genesis definitions
Bothseason_one_endedandseason_two_yield_collectorare defined inproto/noble/dollar/vaults/v1/genesis.proto, have matchingjson:"season_one_ended,omitempty"andjson:"season_two_yield_collector,omitempty"tags intypes/vaults/genesis.pb.go, and are correctly wired ingenesis.go. No changes required.
Yeah @johnletey is not a big deal, just a nit. If we'll have to keep the collector for multiple versions checking it from the code of previous upgrades is not the cleanest thing to do, but as soon as this is going to be the last Vaults upgrade (excluding the final ones) that we'll do it is fine! |
3 participants
Summary by CodeRabbit
New Features
Changes
Chores