DRAFT Deprecation of features#242
Conversation
📝 WalkthroughWalkthroughThe vault API now marks mixed-denom fields, direct-principal RPCs, related messages, and events as deprecated. Generated bindings, specifications, changelog content, and lint exclusions reflect the deprecation guidance and migration toward single-denom and exchange-backed flows. ChangesVault API deprecation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)api/provlabs/vault/v1/tx.pulsar.goast-grep timed out on this file 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. Comment |
|
| Filename | Overview |
|---|---|
| proto/provlabs/vault/v1/tx.proto | Adds deprecation metadata for direct-principal endpoints and mixed-denom fields; the swap-out empty-denom guidance conflicts with the current tx default. |
| proto/provlabs/vault/v1/query.proto | Marks estimate swap-out redeem_denom deprecated; its empty default remains different from the tx path for mixed-denom vaults. |
| proto/provlabs/vault/v1/vault.proto | Marks payment_denom and InitialVaultNAV deprecated while preserving field numbers and compatibility. |
| proto/provlabs/vault/v1/events.proto | Marks direct-principal events deprecated while keeping the event messages available. |
| .golangci.yml | Adds a staticcheck exclusion for internal use of the newly deprecated API surface. |
Reviews (1): Last reviewed commit: "add deprecation" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // - Must be either the vault’s underlying_asset or its payment_denom. | ||
| string redeem_denom = 4; | ||
| // | ||
| // Deprecated: The underlying-or-payment payout choice collapses to the | ||
| // underlying asset only as vaults move to a single denom. Leave this empty; | ||
| // the field will be removed in a future release. | ||
| string redeem_denom = 4 [deprecated = true]; |
There was a problem hiding this comment.
When redeem_denom is omitted for an existing mixed-denom vault, the swap-out tx still defaults to vault.PaymentDenom, but the estimate query defaults to vault.UnderlyingAsset. The new “Leave this empty” guidance can make callers estimate one denom and receive another denom in the actual payout.
| // Deprecated: The underlying-or-payment payout choice collapses to the | ||
| // underlying asset only as vaults move to a single denom. Leave this empty; | ||
| // the field will be removed in a future release. | ||
| string redeem_denom = 3 [deprecated = true]; |
There was a problem hiding this comment.
This comment tells callers to leave redeem_denom empty, and this query then estimates the underlying asset. The matching MsgSwapOut path still pays the payment denom when the same field is omitted, so mixed-denom vault callers can compare an underlying-asset estimate against a payment-denom payout.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.golangci.yml:
- Around line 170-175: The SA1019 exclusion in the golangci configuration is too
broad and may suppress unrelated deprecation warnings. Scope the suppression to
the vault payment-denom and direct-principal symbols or replace it with targeted
//nolint:staticcheck directives at their internal call sites, while preserving
warnings elsewhere.
In `@spec/02_state.md`:
- Line 34: Update the state specification bullet to remove the “must differ from
underlying” requirement for payment denom, and describe it as optional and
deprecated, collapsing to the underlying asset; keep the wording consistent with
the single-denom contract defined by the payment_denom field in vault.proto.
In `@spec/03_messages.md`:
- Around line 133-135: Update the deprecation note for redeem_denom to document
the current legacy behavior: an empty value resolves to the underlying asset for
single-denom vaults, while existing mixed-denom vaults retain the payment_denom
default. Keep the note that the field is deprecated and will be removed in a
future release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c4d98eb-abb5-4745-8cfa-e5cbae49d815
⛔ Files ignored due to path filters (5)
api/provlabs/vault/v1/tx_grpc.pb.gois excluded by!**/*.pb.gotypes/events.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/tx.pb.gois excluded by!**/*.pb.gotypes/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
.changelog/unreleased/deprecated/240-deprecation-sweep.md.golangci.ymlapi/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/query.pulsar.goapi/provlabs/vault/v1/tx.pulsar.goapi/provlabs/vault/v1/vault.pulsar.goproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/query.protoproto/provlabs/vault/v1/tx.protoproto/provlabs/vault/v1/vault.protospec/01_concepts.mdspec/02_state.mdspec/03_messages.mdspec/04_events.mdspec/05_queries.md
| # The module must keep implementing its own deprecated payment-denom and | ||
| # direct-principal surface until it is removed; the deprecation warnings | ||
| # are aimed at external integrators, not this repo's internals. | ||
| - linters: | ||
| - staticcheck | ||
| text: 'SA1019: .*(removed in a future release|removed with the mixed-denom vault concept)' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the SA1019 suppression to this vault surface.
This repository-wide regex can hide unrelated deprecated API usages whenever their deprecation text contains the same phrases. Restrict the rule by path/symbol, or use narrow //nolint:staticcheck directives at the known internal call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.golangci.yml around lines 170 - 175, The SA1019 exclusion in the golangci
configuration is too broad and may suppress unrelated deprecation warnings.
Scope the suppression to the vault payment-denom and direct-principal symbols or
replace it with targeted //nolint:staticcheck directives at their internal call
sites, while preserving warnings elsewhere.
| Each vault is an `x/auth` account implementing `VaultAccountI`. The canonical record contains: | ||
|
|
||
| - Admin address, share denom, underlying asset, optional **payment denom** (must differ from underlying) | ||
| - Admin address, share denom, underlying asset, optional **payment denom** (must differ from underlying; deprecated, collapses to the underlying asset) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Remove the contradictory “must differ” constraint.
proto/provlabs/vault/v1/vault.proto allows payment_denom to be omitted or equal to underlying_asset; only legacy mixed-denom values differ. Update this bullet so it does not contradict the single-denom contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/02_state.md` at line 34, Update the state specification bullet to remove
the “must differ from underlying” requirement for payment denom, and describe it
as optional and deprecated, collapsing to the underlying asset; keep the wording
consistent with the single-denom contract defined by the payment_denom field in
vault.proto.
| > **Deprecated:** `redeem_denom` is deprecated. The underlying-or-payment payout choice collapses | ||
| > to the underlying asset only as vaults move to a single denom. Leave it empty; the field will be | ||
| > removed in a future release. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the current legacy default explicitly.
proto/provlabs/vault/v1/tx.proto:145-189 says an empty redeem_denom currently defaults to the vault’s payment_denom. Clarify that leaving it empty resolves to the underlying asset for single-denom vaults, but retains the payment-denom default on existing mixed-denom vaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/03_messages.md` around lines 133 - 135, Update the deprecation note for
redeem_denom to document the current legacy behavior: an empty value resolves to
the underlying asset for single-denom vaults, while existing mixed-denom vaults
retain the payment_denom default. Keep the note that the field is deprecated and
will be removed in a future release.
|
PR #243 |
1 participant
closes: #240
Summary by CodeRabbit
redeem_denom; leave it empty to receive the underlying asset.