Minimum amounts for swap in and swap out#156
Merged
nullpointer0x00 merged 18 commits intomainfrom Apr 21, 2026
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
keeper/query_server_test.go (1)
23-36:⚠️ Potential issue | 🟡 MinorAssert the new min-swap fields in query responses.
The expected
VaultAccounts now includeMinSwapInValue/MinSwapOutValue, but both manual comparators omit those fields, soVaultandVaultscould stop returning them without failing these tests.Proposed test assertion update
ManualEquality: func(s querytest.TestSuiter, expected, actual *types.QueryVaultResponse) { s.Require().NotNil(actual, "actual response should not be nil") s.Require().NotNil(expected, "expected response should not be nil") s.Assert().Equal(expected.Vault.Address, actual.Vault.Address, "vault address") s.Assert().Equal(expected.Vault.Admin, actual.Vault.Admin, "vault admin") s.Assert().Equal(expected.Vault.TotalShares, actual.Vault.TotalShares, "vault total shares") s.Assert().Equal(expected.Vault.UnderlyingAsset, actual.Vault.UnderlyingAsset, "vault underlying asset") s.Assert().Equal(expected.Vault.AumFeeBips, actual.Vault.AumFeeBips, "vault AUM fee bips") + s.Assert().Equal(expected.Vault.MinSwapInValue, actual.Vault.MinSwapInValue, "vault min swap in value") + s.Assert().Equal(expected.Vault.MinSwapOutValue, actual.Vault.MinSwapOutValue, "vault min swap out value") s.Assert().Equal(expected.Principal.Address, actual.Principal.Address, "principal address") s.Assert().Equal(expected.Principal.Coins, actual.Principal.Coins, "principal coins") s.Assert().Equal(expected.Reserves.Address, actual.Reserves.Address, "reserves address") s.Assert().Equal(expected.Reserves.Coins, actual.Reserves.Coins, "reserves coins") s.Assert().Equal(expected.TotalVaultValue, actual.TotalVaultValue, "total vault value") @@ type vaultView struct { Address string Admin string ShareDenom string UnderlyingAsset string PaymentDenom string AumFeeBips uint32 IsPaused bool + MinSwapInValue string + MinSwapOutValue string } @@ PaymentDenom: v.GetPaymentDenom(), AumFeeBips: v.AumFeeBips, IsPaused: v.GetPaused(), + MinSwapInValue: v.MinSwapInValue, + MinSwapOutValue: v.MinSwapOutValue, })Also applies to: 139-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/query_server_test.go` around lines 23 - 36, The ManualEquality comparator for types.QueryVaultResponse is missing assertions for the new VaultAccount fields MinSwapInValue and MinSwapOutValue; update the ManualEquality function (used with querytest.TestSuiter) to Assert().Equal expected.Vault.MinSwapInValue vs actual.Vault.MinSwapInValue and expected.Vault.MinSwapOutValue vs actual.Vault.MinSwapOutValue (and do the same for any other ManualEquality comparator that compares Vault/Vaults responses) so tests will fail if those fields are omitted or change.keeper/vault.go (1)
111-132:⚠️ Potential issue | 🟠 MajorValidate minimum swap values to prevent negative or malformed values from disabling swap gates.
createVaultAccountpersistsminSwapIn/minSwapOutwithout validation, andSetMinSwapInValue/SetMinSwapOutValueonly check parseability, not whether values are negative. Additionally,AllowSwapInAmountandAllowSwapOutAmountreturntrueon parse failures, silently allowing swaps when configured minimums are malformed. This requires:
- Validating both parseability and non-negative values in all write paths (
createVaultAccountand the setters)- Returning errors (not
true) on invalid persisted state inAllowSwapInAmountandAllowSwapOutAmount- Adding a Godoc comment to the exported
AllowSwapInAmountfunction- Fixing the parameter typo
swapInAssest→swapInAsseton line 549Suggested validation hardening
+func validateMinSwapValue(name, value string) error { + if value == "" { + return nil + } + minLimit, ok := sdkmath.NewIntFromString(value) + if !ok { + return fmt.Errorf("invalid %s: %s", name, value) + } + if minLimit.IsNegative() { + return fmt.Errorf("%s cannot be negative: %s", name, value) + } + return nil +} + func (k *Keeper) createVaultAccount(ctx sdk.Context, admin, shareDenom, underlyingAsset, paymentDenom string, withdrawalDelay uint64, minSwapIn, minSwapOut string) (*types.VaultAccount, error) { vaultAddr := types.GetVaultAddress(shareDenom) + + if err := validateMinSwapValue("min swap in value", minSwapIn); err != nil { + return nil, fmt.Errorf("failed to validate min swap in value: %w", err) + } + if err := validateMinSwapValue("min swap out value", minSwapOut); err != nil { + return nil, fmt.Errorf("failed to validate min swap out value: %w", err) + } params, err := k.Params.Get(ctx)func (k *Keeper) SetMinSwapInValue(ctx sdk.Context, vault *types.VaultAccount, minSwapIn string) error { - if minSwapIn != "" { - if _, ok := sdkmath.NewIntFromString(minSwapIn); !ok { - return fmt.Errorf("invalid min swap in value: %s", minSwapIn) - } + if err := validateMinSwapValue("min swap in value", minSwapIn); err != nil { + return fmt.Errorf("failed to validate min swap in value: %w", err) } if vault.MinSwapInValue == minSwapIn { return nil } @@ func (k *Keeper) SetMinSwapOutValue(ctx sdk.Context, vault *types.VaultAccount, minSwapOut string) error { - if minSwapOut != "" { - if _, ok := sdkmath.NewIntFromString(minSwapOut); !ok { - return fmt.Errorf("invalid min swap out value: %s", minSwapOut) - } + if err := validateMinSwapValue("min swap out value", minSwapOut); err != nil { + return fmt.Errorf("failed to validate min swap out value: %w", err) } if vault.MinSwapOutValue == minSwapOut { return nil }minLimit, ok := sdkmath.NewIntFromString(vault.MinSwapInValue) if !ok { - return true, nil + return false, fmt.Errorf("invalid configured min swap in value: %s", vault.MinSwapInValue) + } + if minLimit.IsNegative() { + return false, fmt.Errorf("configured min swap in value cannot be negative: %s", vault.MinSwapInValue) }minLimit, ok := sdkmath.NewIntFromString(vault.MinSwapOutValue) if !ok { - return true, nil + return false, fmt.Errorf("invalid configured min swap out value: %s", vault.MinSwapOutValue) + } + if minLimit.IsNegative() { + return false, fmt.Errorf("configured min swap out value cannot be negative: %s", vault.MinSwapOutValue) }Also applies to: 514–547, 559–568, 572–591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/vault.go` around lines 111 - 132, The persisted minSwapIn/minSwapOut values must be validated for parseability and non-negativity: update createVaultAccount to parse minSwapIn/minSwapOut as sdk.Int (or the project’s numeric type) and return an error if parsing fails or the value is negative before constructing the VaultAccount; likewise harden SetMinSwapInValue and SetMinSwapOutValue to validate parseability and non-negative values and reject invalid inputs. Modify AllowSwapInAmount and AllowSwapOutAmount to return an error (not silently true) when the stored min values cannot be parsed, and add a Godoc comment to the exported AllowSwapInAmount explaining its behavior and error return. Finally, fix the parameter name typo swapInAssest → swapInAsset in the referenced function signature to match callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changelog/unreleased/features/39-minimums-for-swap-in-out-amounts.md:
- Line 1: The changelog entry incorrectly states both minimum and maximum
limits; update the entry text in
.changelog/unreleased/features/39-minimums-for-swap-in-out-amounts.md to remove
any mention of "maximum" (e.g., change "minimum and maximum limits" to "minimum
limits" or similar), since only min_swap_in_value and min_swap_out_value were
added and no configurable maximum swap amount fields exist (MaxSwapOutBatchSize
is a batch-size constant, not a swap-value limit).
In `@keeper/msg_server_test.go`:
- Around line 968-1202: Extract the repeated vault creation/setup used by
TestMsgServer_UpdateMinSwapInValue, TestMsgServer_UpdateMinSwapInValue_Failures,
TestMsgServer_UpdateMinSwapOutValue, and
TestMsgServer_UpdateMinSwapOutValue_Failures into a helper in suite_test.go
(e.g., CreateAndActivateVault(admin sdk.AccAddress, share string, underlying
string) that calls requireAddFinalizeAndActivateMarker and k.CreateVault and
returns the vault address), then replace the inline setup closures with calls to
that helper; also update the assertions inside the test postCheck and setup code
(the s.Require().NoError(err) and s.Require().Equal(...) calls in the postCheck
closures and setup) to include descriptive failure messages (e.g., "unexpected
error creating vault", "vault MinSwapInValue mismatch: expected %s got %s",
"vault MinSwapOutValue mismatch: expected %s got %s") so every Require has
contextual text. Ensure you reference
keeper.NewMsgServer(...).UpdateMinSwapInValue, UpdateMinSwapOutValue,
requireAddFinalizeAndActivateMarker, and k.CreateVault when making the changes.
In `@keeper/vault.go`:
- Around line 549-552: Add a proper Godoc comment for the exported method
AllowSwapInAmount on Keeper and rename the misspelled parameter swapInAssest to
swapInAsset in its signature and all usages (including calls to
ToUnderlyingAssetAmount and any interfaces/tests) so the public API no longer
exposes the typo; update the function comment to describe purpose, parameters
(ctx, swapInAsset, vault types.VaultAccount) and return values, and run/adjust
any callers or generated docs that reference the old name.
In `@proto/provlabs/vault/v1/events.proto`:
- Around line 381-399: Update the proto docs for EventMinSwapInValueUpdated and
EventMinSwapOutValueUpdated to explicitly state the "clear-minimum" semantics
for the min_swap_in and min_swap_out fields: document that an empty string ("")
or "0" indicates the minimum was cleared / no configured minimum, and align the
wording with the tx/state scalar metadata conventions; update the comment blocks
on the min_swap_in and min_swap_out fields in those message definitions
(EventMinSwapInValueUpdated, EventMinSwapOutValueUpdated) to include this
behavior so event consumers and generated API docs unambiguously understand the
cleared-minimum semantics.
In `@proto/provlabs/vault/v1/tx.proto`:
- Around line 120-123: Update the CreateVault message documentation for the
fields min_swap_in_value and min_swap_out_value to state the no-minimum
semantics consistently with the update messages: clearly document that setting
the field to "" (empty string) or "0" disables the minimum, indicate the unit
(underlying_asset) and that values are encoded as cosmos.IntString, and include
examples or allowed formats if helpful; modify the comments above string
min_swap_in_value and string min_swap_out_value to reflect this behavior so the
public API docs match the update messages.
- Around line 42-46: The proto currently only exposes UpdateMinSwapInValue and
UpdateMinSwapOutValue RPCs; add symmetric maximum-value endpoints and messages
by introducing rpc MsgUpdateMaxSwapInValue(MsgUpdateMaxSwapInValueRequest)
returns (MsgUpdateMaxSwapInValueResponse) and rpc
MsgUpdateMaxSwapOutValue(MsgUpdateMaxSwapOutValueRequest) returns
(MsgUpdateMaxSwapOutValueResponse) in proto/provlabs/vault/v1/tx.proto, define
the corresponding request/response message types (including validator/owner, max
value field, and standard Msg fields), and implement the server-side handlers
(e.g., MsgServer methods named UpdateMaxSwapInValue and UpdateMaxSwapOutValue
and their keeper/handler logic) to persist and validate the configured maximums
consistent with existing UpdateMin* implementations.
In `@proto/provlabs/vault/v1/vault.proto`:
- Around line 119-129: The comments for the proto fields min_swap_in_value and
min_swap_out_value lack an explicit non-negative invariant; update the
documentation in vault.proto to state that these fields are integer strings
(cosmos.IntString) and must be non-negative (>= 0), e.g., clarify that values
are measured in underlying_asset, inputs are converted before checking, and that
the empty string or "0" disables the minimum; ensure both min_swap_in_value and
min_swap_out_value documentation include the non-negative requirement so clients
cannot assume signed values are allowed.
In `@spec/03_messages.md`:
- Around line 74-77: Update the spec text for MsgCreateVaultRequest (and the
analogous update message around the second range) to state that
min_swap_in_value and min_swap_out_value are denominated in the vault's
underlying_asset and that sending an empty string ("") or the string "0"
indicates clearing/disabling the minimum; reference the exact fields by name
(MsgCreateVaultRequest.admin, MsgCreateVaultRequest.share_denom,
MsgCreateVaultRequest.underlying_asset, MsgCreateVaultRequest.payment_denom,
MsgCreateVaultRequest.withdrawal_delay_seconds,
MsgCreateVaultRequest.min_swap_in_value,
MsgCreateVaultRequest.min_swap_out_value) and mirror the proto semantics so
clients know to express amounts in underlying_asset units and to use "" or "0"
to clear the minimums.
In `@types/msgs.go`:
- Around line 78-88: The current validation for m.MinSwapInValue and
m.MinSwapOutValue uses sdkmath.NewIntFromString but permits negative integers;
update the validators (e.g., in the message ValidateBasic function handling
m.MinSwapInValue and m.MinSwapOutValue) to parse the string with
sdkmath.NewIntFromString and then check the resulting Int is >= 0 (treat "" and
"0" as valid), returning an error like "invalid min swap in value: must be
non-negative: %q" (or similar) if the value is negative; apply the same
non-negative check to the other similar fields noted (the block around lines
481-499) so all min-swap thresholds reject negative values.
In `@types/vault.go`:
- Around line 209-219: The current validation uses sdkmath.NewIntFromString for
v.MinSwapInValue and v.MinSwapOutValue but doesn't reject negative values;
update the validation in the same blocks that call sdkmath.NewIntFromString (for
v.MinSwapInValue and v.MinSwapOutValue) to parse the value and then check that
the resulting Int is >= 0, returning an error like "min swap in/out value must
be non-negative" if it's negative so that AllowSwapInAmount and
AllowSwapOutAmount comparisons cannot be bypassed by negative minima.
---
Outside diff comments:
In `@keeper/query_server_test.go`:
- Around line 23-36: The ManualEquality comparator for types.QueryVaultResponse
is missing assertions for the new VaultAccount fields MinSwapInValue and
MinSwapOutValue; update the ManualEquality function (used with
querytest.TestSuiter) to Assert().Equal expected.Vault.MinSwapInValue vs
actual.Vault.MinSwapInValue and expected.Vault.MinSwapOutValue vs
actual.Vault.MinSwapOutValue (and do the same for any other ManualEquality
comparator that compares Vault/Vaults responses) so tests will fail if those
fields are omitted or change.
In `@keeper/vault.go`:
- Around line 111-132: The persisted minSwapIn/minSwapOut values must be
validated for parseability and non-negativity: update createVaultAccount to
parse minSwapIn/minSwapOut as sdk.Int (or the project’s numeric type) and return
an error if parsing fails or the value is negative before constructing the
VaultAccount; likewise harden SetMinSwapInValue and SetMinSwapOutValue to
validate parseability and non-negative values and reject invalid inputs. Modify
AllowSwapInAmount and AllowSwapOutAmount to return an error (not silently true)
when the stored min values cannot be parsed, and add a Godoc comment to the
exported AllowSwapInAmount explaining its behavior and error return. Finally,
fix the parameter name typo swapInAssest → swapInAsset in the referenced
function signature to match callers.
🪄 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: 704aa915-d0a5-4829-9783-b80dbac7c8d6
⛔ Files ignored due to path filters (4)
api/provlabs/vault/v1/tx_grpc.pb.gois excluded by!**/*.pb.gotypes/events.pb.gois excluded by!**/*.pb.gotypes/tx.pb.gois excluded by!**/*.pb.gotypes/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
.changelog/unreleased/features/39-minimums-for-swap-in-out-amounts.mdapi/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/tx.pulsar.goapi/provlabs/vault/v1/vault.pulsar.gokeeper/msg_server.gokeeper/msg_server_test.gokeeper/query_server_test.gokeeper/suite_test.gokeeper/vault.goproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/tx.protoproto/provlabs/vault/v1/vault.protospec/02_state.mdspec/03_messages.mdtypes/events.gotypes/msgs.gotypes/vault.go
Taztingo
reviewed
Apr 20, 2026
Taztingo
reviewed
Apr 20, 2026
Collaborator
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
simulation/operations.go (2)
640-657:⚠️ Potential issue | 🟡 MinorUse the selected accepted asset when constructing principal deposits.
Line 640 may select
payment_denom, but Line 657 always creates anunderlying_assetcoin. That can base the amount on one balance and attempt to send another denom.Proposed fix
- amount := sdk.NewCoin(vault.UnderlyingAsset, amountInt) + amount := sdk.NewCoin(asset, amountInt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/operations.go` around lines 640 - 657, The code builds the deposit coin with vault.UnderlyingAsset even though you previously selected and used a random asset (asset) for balance checks; update the creation of the deposit coin to use the selected asset's denom so the coin's denom matches the balance you sampled. Specifically, in the block around getRandomVaultAsset, balance, portion and amountInt, replace the sdk.NewCoin(vault.UnderlyingAsset, amountInt) construction with sdk.NewCoin(asset.Denom, amountInt) (or the appropriate field name on the selected asset) so the deposit uses the same denom as the sampled balance.
694-700:⚠️ Potential issue | 🟡 MinorUse the selected accepted asset when constructing principal withdrawals.
Line 695 may choose
payment_denom, but Line 700 always withdrawsunderlying_asset; this can make the simulation no-op even when the selected accepted asset has funds.Proposed fix
- amount := sdk.NewInt64Coin(vault.UnderlyingAsset, r.Int63n(balance.Amount.Int64())) + amount := sdk.NewInt64Coin(asset, r.Int63n(balance.Amount.Int64()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/operations.go` around lines 694 - 700, The simulation is picking a random accepted asset via getRandomVaultAsset but then always builds the withdrawal coin using vault.UnderlyingAsset, which can cause a no-op; change the coin construction to use the selected asset denomination (the variable asset) and its balance when creating amount in the principal withdrawal flow (around principalAddr, getRandomVaultAsset, balance, amount and MsgWithdrawPrincipalFundsRequest) so the sdk.NewCoin/ NewInt64Coin uses asset as the denom and the random amount is based on balance.Amount.keeper/msg_server_test.go (2)
722-782:⚠️ Potential issue | 🟡 MinorMint enough shares for the swap-out max-limit case.
The “above maximum” case burns
150_000_000shares, but setup mints only100_000_000. Increase the setup deposit or top up that case so the expected failure is only the max-limit violation, not insufficient shares.Proposed test isolation fix
- initialAssets := sdk.NewInt64Coin(underlyingDenom, 100) + initialAssets := sdk.NewInt64Coin(underlyingDenom, 150)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/msg_server_test.go` around lines 722 - 782, The "above maximum" test currently burns 150_000_000 shares but the setup only mints 100_000_000 via initialAssets; adjust the setup so the vault mints at least 150_000_000 shares (e.g., increase initialAssets or add a top-up FundAccount + SwapIn for the specific test case) so the failure is due solely to the MaxSwapOutValue check; update the test case that calls setup(true, false, "", "100") or add a per-test FundAccount/SwapIn using vaultAddr and owner to ensure sufficient mintedShares before invoking MsgSwapOutRequest.
491-541:⚠️ Potential issue | 🟡 MinorIsolate the swap-in max-limit assertion from balance failures.
The “above maximum” case sends
150underlying, but setup funds the owner with onlyassets(100). This makes the test depend on max-limit validation running before balance validation.Proposed test isolation fix
- assets := sdk.NewInt64Coin(underlyingDenom, 100) + assets := sdk.NewInt64Coin(underlyingDenom, 150)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/msg_server_test.go` around lines 491 - 541, The "above maximum" test relies on owner having only `assets` (100) which causes a balance failure before the vault max-limit check; update the test setup so the owner is funded with more than the attempted swap amount for that case (e.g., fund with >=150) or override the `assets` used in FundAccount for the specific "swap in above maximum value" case so the MsgSwapInRequest (types.MsgSwapInRequest) with 150 triggers the vault max validation rather than a balance error; locate the closure `setup(swapInEnabled, vaultPaused bool, minSwapIn, maxSwapIn string)` and the test entry in `tests := []msgServerTestCase[...]` to apply this change.
♻️ Duplicate comments (1)
proto/provlabs/vault/v1/tx.proto (1)
126-133:⚠️ Potential issue | 🟡 MinorDocument create-time clear/block semantics for all swap limits.
These
CreateVaultfields should mirror the update-message docs: non-negative values,""/"0"clears minimums,""clears maximums, and"0"blocks swap-in/out for maximums.Suggested proto doc update
- // min_swap_in_value is the minimum value required for a deposit, measured in the underlying_asset. + // min_swap_in_value is the minimum value required for a deposit, measured in the underlying_asset. + // - Values must be non-negative (>= 0). + // - An empty string "" or "0" represents no minimum. string min_swap_in_value = 6 [(cosmos_proto.scalar) = "cosmos.IntString"]; - // min_swap_out_value is the minimum value required for a withdrawal, measured in the underlying_asset. + // min_swap_out_value is the minimum value required for a withdrawal, measured in the underlying_asset. + // - Values must be non-negative (>= 0). + // - An empty string "" or "0" represents no minimum. string min_swap_out_value = 7 [(cosmos_proto.scalar) = "cosmos.IntString"]; - // max_swap_in_value is the maximum value allowed for a deposit, measured in the underlying_asset. + // max_swap_in_value is the maximum value allowed for a deposit, measured in the underlying_asset. + // - Values must be non-negative (>= 0). + // - An empty string "" represents no maximum. + // - A value of "0" blocks all deposits. string max_swap_in_value = 8 [(cosmos_proto.scalar) = "cosmos.IntString"]; - // max_swap_out_value is the maximum value allowed for a withdrawal, measured in the underlying_asset. + // max_swap_out_value is the maximum value allowed for a withdrawal, measured in the underlying_asset. + // - Values must be non-negative (>= 0). + // - An empty string "" represents no maximum. + // - A value of "0" blocks all withdrawals. string max_swap_out_value = 9 [(cosmos_proto.scalar) = "cosmos.IntString"];As per coding guidelines,
proto/**/*.proto: “All fields and messages in.protofiles must be thoroughly documented, as these propagate to generated code and public API specs”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/provlabs/vault/v1/tx.proto` around lines 126 - 133, Create-time semantics for the CreateVault swap limit fields (min_swap_in_value, min_swap_out_value, max_swap_in_value, max_swap_out_value) are undocumented; update the proto comments for these fields in the CreateVault message to mirror the update-message docs: state that values must be non-negative, that an empty string "" or "0" clears minimums as specified, that "" clears maximums and "0" blocks swap-in/out for maximums, and include explicit examples for clearing vs blocking; reference the exact field names (min_swap_in_value, min_swap_out_value, max_swap_in_value, max_swap_out_value) and mention CreateVault so readers can find the fields.
🧹 Nitpick comments (2)
simulation/rand.go (1)
416-436: Drop the unused keeper parameter from the helper.
k keeper.Keeperis not used ingetRandomManagementAuthority; removing it will simplify every call site.Small cleanup
-func getRandomManagementAuthority(r *rand.Rand, k keeper.Keeper, ctx sdk.Context, vault *types.VaultAccount, accs []simtypes.Account) (simtypes.Account, error) { +func getRandomManagementAuthority(r *rand.Rand, ctx sdk.Context, vault *types.VaultAccount, accs []simtypes.Account) (simtypes.Account, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/rand.go` around lines 416 - 436, The function getRandomManagementAuthority currently accepts an unused parameter k of type keeper.Keeper; remove this parameter from the function signature (getRandomManagementAuthority(r *rand.Rand, ctx sdk.Context, vault *types.VaultAccount, accs []simtypes.Account) ...) and update every call site to stop passing the keeper; also update any tests or helper callers and imports if necessary so builds pass after deleting the unused k argument.keeper/msg_server_test.go (1)
1118-1148: Add negative-value rows for the new limit update endpoints.The failure tables cover non-numeric values, but
"-1"is a separate edge case: it can parse while still being an invalid limit. Please add negative cases for min/max swap-in/out updates to lock in the “no negative limits” behavior.As per coding guidelines, "
**/*_test.go: Table-driven tests are mandatory for unit and integration tests to ensure exhaustive coverage of edge cases."Also applies to: 1257-1287, 1396-1426, 1535-1565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/msg_server_test.go` around lines 1118 - 1148, Add table rows that assert negative numeric inputs are rejected: for the tests slice of msgServerTestCase for MsgUpdateMinSwapInValueRequest, add a case with MinSwapInValue set to "-1" (Authority admin, VaultAddress types.GetVaultAddress(share).String()) and an expectedErrSubstrs entry indicating the "negative" or "no negative limits" rejection (e.g., "negative" or "invalid min swap in value" depending on existing error messages). Repeat the same pattern for the other limit-update test tables referenced (the Min/Max swap-in/out update tables around the other noted blocks) so each has an explicit negative-value test row verifying the “no negative limits” behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/vault.go`:
- Around line 530-616: The setters (SetMinSwapInValue, SetMinSwapOutValue,
SetMaxSwapInValue, SetMaxSwapOutValue) only validate their single argument,
allowing admins to create invalid ranges (min > max); update each setter to
check the new value against the opposite bound stored on vault before calling
SetVaultAccount—e.g., in SetMinSwapInValue ensure new min <= existing
MaxSwapInValue (unless MaxSwapInValue == "0" which should remain a valid “block
all” sentinel), and similarly validate SetMinSwapOutValue against
MaxSwapOutValue, SetMaxSwapInValue against MinSwapInValue, and
SetMaxSwapOutValue against MinSwapOutValue; if the pair is invalid return an
error and do not persist, mirroring the interest-rate validation pattern already
used elsewhere.
- Around line 625-637: Parsing of stored limits (MinSwapInValue/MaxSwapInValue)
currently ignores NewIntFromString failures and treats "0" max as allowing a
zero swap; change both AllowSwapInAmount and AllowSwapOutAmount to fail-closed:
attempt NewIntFromString for MinSwapInValue/MaxSwapInValue and if parsing
returns !ok return false with a parse error message; after successful parse, for
min check the existing LT(minLimit) behavior, and for max treat a parsed
maxLimit that .IsZero() as blocking all swaps (return false) rather than
allowing equality — i.e., return false immediately if maxLimit.IsZero(),
otherwise keep the GT(maxLimit) check; apply identical parsing+fail-closed logic
for both min and max in the referenced functions.
---
Outside diff comments:
In `@keeper/msg_server_test.go`:
- Around line 722-782: The "above maximum" test currently burns 150_000_000
shares but the setup only mints 100_000_000 via initialAssets; adjust the setup
so the vault mints at least 150_000_000 shares (e.g., increase initialAssets or
add a top-up FundAccount + SwapIn for the specific test case) so the failure is
due solely to the MaxSwapOutValue check; update the test case that calls
setup(true, false, "", "100") or add a per-test FundAccount/SwapIn using
vaultAddr and owner to ensure sufficient mintedShares before invoking
MsgSwapOutRequest.
- Around line 491-541: The "above maximum" test relies on owner having only
`assets` (100) which causes a balance failure before the vault max-limit check;
update the test setup so the owner is funded with more than the attempted swap
amount for that case (e.g., fund with >=150) or override the `assets` used in
FundAccount for the specific "swap in above maximum value" case so the
MsgSwapInRequest (types.MsgSwapInRequest) with 150 triggers the vault max
validation rather than a balance error; locate the closure `setup(swapInEnabled,
vaultPaused bool, minSwapIn, maxSwapIn string)` and the test entry in `tests :=
[]msgServerTestCase[...]` to apply this change.
In `@simulation/operations.go`:
- Around line 640-657: The code builds the deposit coin with
vault.UnderlyingAsset even though you previously selected and used a random
asset (asset) for balance checks; update the creation of the deposit coin to use
the selected asset's denom so the coin's denom matches the balance you sampled.
Specifically, in the block around getRandomVaultAsset, balance, portion and
amountInt, replace the sdk.NewCoin(vault.UnderlyingAsset, amountInt)
construction with sdk.NewCoin(asset.Denom, amountInt) (or the appropriate field
name on the selected asset) so the deposit uses the same denom as the sampled
balance.
- Around line 694-700: The simulation is picking a random accepted asset via
getRandomVaultAsset but then always builds the withdrawal coin using
vault.UnderlyingAsset, which can cause a no-op; change the coin construction to
use the selected asset denomination (the variable asset) and its balance when
creating amount in the principal withdrawal flow (around principalAddr,
getRandomVaultAsset, balance, amount and MsgWithdrawPrincipalFundsRequest) so
the sdk.NewCoin/ NewInt64Coin uses asset as the denom and the random amount is
based on balance.Amount.
---
Duplicate comments:
In `@proto/provlabs/vault/v1/tx.proto`:
- Around line 126-133: Create-time semantics for the CreateVault swap limit
fields (min_swap_in_value, min_swap_out_value, max_swap_in_value,
max_swap_out_value) are undocumented; update the proto comments for these fields
in the CreateVault message to mirror the update-message docs: state that values
must be non-negative, that an empty string "" or "0" clears minimums as
specified, that "" clears maximums and "0" blocks swap-in/out for maximums, and
include explicit examples for clearing vs blocking; reference the exact field
names (min_swap_in_value, min_swap_out_value, max_swap_in_value,
max_swap_out_value) and mention CreateVault so readers can find the fields.
---
Nitpick comments:
In `@keeper/msg_server_test.go`:
- Around line 1118-1148: Add table rows that assert negative numeric inputs are
rejected: for the tests slice of msgServerTestCase for
MsgUpdateMinSwapInValueRequest, add a case with MinSwapInValue set to "-1"
(Authority admin, VaultAddress types.GetVaultAddress(share).String()) and an
expectedErrSubstrs entry indicating the "negative" or "no negative limits"
rejection (e.g., "negative" or "invalid min swap in value" depending on existing
error messages). Repeat the same pattern for the other limit-update test tables
referenced (the Min/Max swap-in/out update tables around the other noted blocks)
so each has an explicit negative-value test row verifying the “no negative
limits” behavior.
In `@simulation/rand.go`:
- Around line 416-436: The function getRandomManagementAuthority currently
accepts an unused parameter k of type keeper.Keeper; remove this parameter from
the function signature (getRandomManagementAuthority(r *rand.Rand, ctx
sdk.Context, vault *types.VaultAccount, accs []simtypes.Account) ...) and update
every call site to stop passing the keeper; also update any tests or helper
callers and imports if necessary so builds pass after deleting the unused k
argument.
🪄 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: aacaf6c6-cb79-4ee2-8d79-a3e50ebc1ad0
⛔ Files ignored due to path filters (4)
api/provlabs/vault/v1/tx_grpc.pb.gois excluded by!**/*.pb.gotypes/events.pb.gois excluded by!**/*.pb.gotypes/tx.pb.gois excluded by!**/*.pb.gotypes/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
api/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/tx.pulsar.goapi/provlabs/vault/v1/vault.pulsar.gokeeper/msg_server.gokeeper/msg_server_test.gokeeper/query_server_test.gokeeper/suite_test.gokeeper/vault.gomodule.goproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/tx.protoproto/provlabs/vault/v1/vault.protosimulation/operations.gosimulation/rand.gospec/02_state.mdspec/03_messages.mdtypes/events.gotypes/msgs.gotypes/vault.go
🚧 Files skipped from review as they are similar to previous changes (8)
- spec/02_state.md
- keeper/query_server_test.go
- proto/provlabs/vault/v1/vault.proto
- types/vault.go
- proto/provlabs/vault/v1/events.proto
- spec/03_messages.md
- types/msgs.go
- keeper/suite_test.go
Collaborator
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
Taztingo
reviewed
Apr 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
spec/03_messages.md (1)
78-81:⚠️ Potential issue | 🟡 MinorMirror the proto swap-limit semantics in the spec.
Line 78 and the new update sections don’t tell clients that all swap-limit values are measured in the vault’s
underlying_asset; the CreateVault section also omits the min/max clearing rules and max positivity constraint.Suggested docs update
-Creates a new vault account with a configured underlying asset, optional payment denom, withdrawal delay, and minimum/maximum swap values. +Creates a new vault account with a configured underlying asset, optional payment denom, withdrawal delay, and optional minimum/maximum swap values measured in the vault’s `underlying_asset`. +For `min_swap_in_value` and `min_swap_out_value`, an empty string (`""`) or `"0"` means no minimum. +For `max_swap_in_value` and `max_swap_out_value`, an empty string (`""`) means no maximum; set values must be positive (`> 0`). @@ -Admin or Asset Manager. Updates the minimum allowed value for a swap-in operation. +Admin or Asset Manager. Updates the minimum allowed value for a swap-in operation, measured in the vault’s `underlying_asset`. @@ -Admin or Asset Manager. Updates the minimum allowed value for a swap-out operation. +Admin or Asset Manager. Updates the minimum allowed value for a swap-out operation, measured in the vault’s `underlying_asset`. @@ -Admin or Asset Manager. Updates the maximum allowed value for a swap-in operation. +Admin or Asset Manager. Updates the maximum allowed value for a swap-in operation, measured in the vault’s `underlying_asset`. @@ -Admin or Asset Manager. Updates the maximum allowed value for a swap-out operation. +Admin or Asset Manager. Updates the maximum allowed value for a swap-out operation, measured in the vault’s `underlying_asset`.Also applies to: 153-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/03_messages.md` around lines 78 - 81, The spec's MsgCreateVaultRequest and related update sections must explicitly state that all swap-limit fields (min_swap_in_value, min_swap_out_value, max_swap_in_value, max_swap_out_value) are denominated in the vault's underlying_asset, and must document the same min/max clearing rules and the "max must be positive" constraint used in the proto: update the CreateVault description for MsgCreateVaultRequest and the corresponding update sections to (1) state units = underlying_asset, (2) explain that unspecified min values clear to zero and unspecified max values clear/unset, and (3) assert that any provided max_swap_* value must be > 0; reference MsgCreateVaultRequest and the swap-limit field names so clients can locate and apply the same semantics across the spec.
🧹 Nitpick comments (1)
keeper/vault.go (1)
532-533: Wrap swap-limit validation errors with setter context.The new setters return
types.ValidateSwapLimitserrors directly, unlike the surrounding setters that add operation context before returning.Proposed cleanup
if err := types.ValidateSwapLimits(minSwapIn, vault.MaxSwapInValue, true); err != nil { - return err + return fmt.Errorf("failed to validate swap-in value limits: %w", err) } @@ if err := types.ValidateSwapLimits(minSwapOut, vault.MaxSwapOutValue, false); err != nil { - return err + return fmt.Errorf("failed to validate swap-out value limits: %w", err) } @@ if err := types.ValidateSwapLimits(vault.MinSwapInValue, maxSwapIn, true); err != nil { - return err + return fmt.Errorf("failed to validate swap-in value limits: %w", err) } @@ if err := types.ValidateSwapLimits(vault.MinSwapOutValue, maxSwapOut, false); err != nil { - return err + return fmt.Errorf("failed to validate swap-out value limits: %w", err) }As per coding guidelines, "
**/*.go: Always wrap errors with context usingfmt.Errorf(\"failed to [action]: %w\", err)pattern."Also applies to: 548-549, 564-565, 580-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/vault.go` around lines 532 - 533, The calls to types.ValidateSwapLimits (e.g., the invocation using minSwapIn and vault.MaxSwapInValue) return errors directly; wrap each returned error with contextual setter information using fmt.Errorf("failed to set [field]: %w", err) before returning so the caller knows which setter failed. Update the specific ValidateSwapLimits call shown and the other occurrences referenced (the calls at the other three sites) to return a wrapped error that names the setter operation (for example "failed to set MinSwapIn" or the appropriate field name) while preserving the original error via %w.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@types/msgs_test.go`:
- Around line 1615-1805: The test tables for MsgUpdate* swap-limit validations
are asymmetric and missing invalid VaultAddress, invalid authority and
non-integer cases across MsgUpdateMinSwapOutValueRequest,
MsgUpdateMaxSwapInValueRequest, and MsgUpdateMaxSwapOutValueRequest, and the
assertion blocks lack descriptive messages; update each of the four table-driven
tests (tables for MsgUpdateMinSwapInValueRequest,
MsgUpdateMinSwapOutValueRequest, MsgUpdateMaxSwapInValueRequest,
MsgUpdateMaxSwapOutValueRequest) to include the same edge cases: invalid
VaultAddress, invalid authority, non-integer value, negative value and
zero-if-blocked as applicable, and then replace the repeated assertion block
that calls ValidateBasic() with a single shared assertion that includes a
descriptive failure message (e.g., include tt.name and expectedErr in the
assert.NoError/assert.Error/assert.Contains messages) so failures self-diagnose;
ensure you reference and update the ValidateBasic() checks for those message
structs accordingly.
In `@types/msgs.go`:
- Around line 42-43: AllRequestMsgs is missing the new max-swap update message
types, causing asymmetry with the min-swap entries; add
(*MsgUpdateMaxSwapInValueRequest)(nil) and
(*MsgUpdateMaxSwapOutValueRequest)(nil) to the AllRequestMsgs slice/registry so
the max update messages are registered alongside
(*MsgUpdateMinSwapInValueRequest)(nil) and
(*MsgUpdateMinSwapOutValueRequest)(nil), ensuring downstream message/interface
registration sees the new types.
In `@types/vault_test.go`:
- Around line 884-889: The assertions around tt.expectedErr and err lack
descriptive failure messages; update the assert calls (assert.NoError,
assert.Error, assert.Contains) to include a clear message that references the
test case (e.g., tt.name or similar), the expected error (tt.expectedErr) and
the actual error (err) so failures show context and expected vs actual; locate
the assertion block using symbols tt.expectedErr, err,
assert.NoError/assert.Error/assert.Contains and add formatted messages via t or
fmt.Sprintf to each assertion call.
In `@types/vault.go`:
- Around line 71-72: Update the Godoc for NewVaultAccount to reflect current
swap-limit semantics and to document the new swap-limit parameters: explain that
maxSwapInValue/maxSwapOutValue of "0" are rejected by validation (callers should
toggle messages instead), and describe the purpose and units/format of
minSwapInValue, minSwapOutValue, maxSwapInValue, and maxSwapOutValue; reference
the NewVaultAccount signature and mention withdrawalDelay and aumFeeBips briefly
to keep context about related parameters.
---
Duplicate comments:
In `@spec/03_messages.md`:
- Around line 78-81: The spec's MsgCreateVaultRequest and related update
sections must explicitly state that all swap-limit fields (min_swap_in_value,
min_swap_out_value, max_swap_in_value, max_swap_out_value) are denominated in
the vault's underlying_asset, and must document the same min/max clearing rules
and the "max must be positive" constraint used in the proto: update the
CreateVault description for MsgCreateVaultRequest and the corresponding update
sections to (1) state units = underlying_asset, (2) explain that unspecified min
values clear to zero and unspecified max values clear/unset, and (3) assert that
any provided max_swap_* value must be > 0; reference MsgCreateVaultRequest and
the swap-limit field names so clients can locate and apply the same semantics
across the spec.
---
Nitpick comments:
In `@keeper/vault.go`:
- Around line 532-533: The calls to types.ValidateSwapLimits (e.g., the
invocation using minSwapIn and vault.MaxSwapInValue) return errors directly;
wrap each returned error with contextual setter information using
fmt.Errorf("failed to set [field]: %w", err) before returning so the caller
knows which setter failed. Update the specific ValidateSwapLimits call shown and
the other occurrences referenced (the calls at the other three sites) to return
a wrapped error that names the setter operation (for example "failed to set
MinSwapIn" or the appropriate field name) while preserving the original error
via %w.
🪄 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: 14c0c2c4-40e9-41a9-a4c1-c084bf39d1cf
⛔ Files ignored due to path filters (3)
types/events.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)
api/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/tx.pulsar.goapi/provlabs/vault/v1/vault.pulsar.gokeeper/msg_server_test.gokeeper/vault.goproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/tx.protoproto/provlabs/vault/v1/vault.protosimulation/operations.gosimulation/rand.gospec/03_messages.mdtypes/msgs.gotypes/msgs_test.gotypes/vault.gotypes/vault_test.go
✅ Files skipped from review due to trivial changes (2)
- proto/provlabs/vault/v1/events.proto
- api/provlabs/vault/v1/vault.pulsar.go
🚧 Files skipped from review as they are similar to previous changes (1)
- proto/provlabs/vault/v1/vault.proto
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Taztingo
approved these changes
Apr 21, 2026
nullpointer0x00
deleted the
nullpointer0x00/39-minimums-for-swap-in-out-amounts
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #39
Summary by CodeRabbit
New Features
Documentation
Tests