-
Notifications
You must be signed in to change notification settings - Fork 886
fix(feegrant): bound DenomsSubsetOf cost and cap allowance denoms #3710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
21d391c
341afd2
0814429
5586dbb
b40c6f4
a15ece7
bba284d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -358,16 +358,30 @@ func (coins Coins) safeAdd(coinsB Coins) Coins { | |
|
|
||
| // DenomsSubsetOf returns true if receiver's denom set | ||
| // is subset of coinsB's denoms. | ||
| // | ||
| // Both coin sets — the receiver and coinsB — are assumed to be sorted ascending | ||
| // by denomination and free of zero amounts (the Coins invariant enforced by | ||
| // Validate/IsValid). The receiver must be sorted too: the merge walk only | ||
| // advances forward through coinsB, so an unsorted receiver can produce a false | ||
| // negative. Under the sorted invariant a denom is "present" iff it appears, so | ||
| // this merge walk can run in O(len(coins) + len(coinsB)). | ||
| func (coins Coins) DenomsSubsetOf(coinsB Coins) bool { | ||
| // more denoms in B than in receiver | ||
| // more denoms in receiver than in B => cannot be a subset | ||
| if len(coins) > len(coinsB) { | ||
| return false | ||
| } | ||
|
|
||
| indexB := 0 | ||
| for _, coin := range coins { | ||
| if coinsB.AmountOf(coin.Denom).IsZero() { | ||
| // advance B until it reaches or passes coin's denom | ||
| for indexB < len(coinsB) && coinsB[indexB].Denom < coin.Denom { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This merge walk now requires the receiver ( |
||
| indexB++ | ||
| } | ||
| if indexB == len(coinsB) || coinsB[indexB].Denom != coin.Denom { | ||
| return false | ||
| } | ||
| // matched; both lists are duplicate-free so B never re-matches this denom | ||
| indexB++ | ||
| } | ||
|
|
||
| return true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,10 @@ import ( | |
|
|
||
| var _ FeeAllowanceI = (*BasicAllowance)(nil) | ||
|
|
||
| // MaxAllowanceDenoms bounds the number of coins allowed in a single allowance | ||
| // spend-limit list. | ||
| const MaxAllowanceDenoms = 100 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocker] |
||
|
|
||
| // Accept can use fee payment requested as well as timestamp of the current block | ||
| // to determine whether or not to process this. This is checked in | ||
| // Keeper.UseGrantedFees and the return values should match how it is handled there. | ||
|
|
@@ -38,6 +42,9 @@ func (a *BasicAllowance) Accept(ctx sdk.Context, fee sdk.Coins, _ []sdk.Msg) (bo | |
| // ValidateBasic implements FeeAllowance and enforces basic sanity checks | ||
| func (a BasicAllowance) ValidateBasic() error { | ||
| if a.SpendLimit != nil { | ||
| if len(a.SpendLimit) > MaxAllowanceDenoms { | ||
| return sdkerrors.Wrapf(ErrTooManyDenoms, "spend limit has %d denoms, max %d", len(a.SpendLimit), MaxAllowanceDenoms) | ||
| } | ||
| if !a.SpendLimit.IsValid() { | ||
| return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "send amount is invalid: %s", a.SpendLimit) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package feegrant_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
| "time" | ||
|
|
||
|
|
@@ -148,3 +149,62 @@ func TestBasicFeeValidAllow(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| // sortedCoins builds a valid, sorted, duplicate-free Coins list of n denoms | ||
| // (zero-padded so lexical order matches numeric order). | ||
| func sortedCoins(n int) sdk.Coins { | ||
| coins := make(sdk.Coins, n) | ||
| for i := 0; i < n; i++ { | ||
| coins[i] = sdk.NewInt64Coin(fmt.Sprintf("coin%08d", i), 1) | ||
| } | ||
| return coins.Sort() | ||
| } | ||
|
|
||
| func TestBasicAllowanceMaxDenoms(t *testing.T) { | ||
| atCap := &feegrant.BasicAllowance{SpendLimit: sortedCoins(feegrant.MaxAllowanceDenoms)} | ||
| require.NoError(t, atCap.ValidateBasic()) | ||
|
|
||
| overCap := &feegrant.BasicAllowance{SpendLimit: sortedCoins(feegrant.MaxAllowanceDenoms + 1)} | ||
| err := overCap.ValidateBasic() | ||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, feegrant.ErrTooManyDenoms) | ||
| } | ||
|
|
||
| func TestPeriodicAllowanceMaxDenoms(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This test doesn't actually exercise the new |
||
| atCap := sortedCoins(feegrant.MaxAllowanceDenoms) | ||
| over := sortedCoins(feegrant.MaxAllowanceDenoms + 1) | ||
|
|
||
| // Each case keeps the other allowance fields under cap so that a single | ||
| // over-cap field is the only thing that can trip ErrTooManyDenoms. This | ||
| // isolates the periodic-specific checks; if they only oversized every field | ||
| // at once, the embedded Basic.ValidateBasic() would short-circuit and the | ||
| // periodic checks would never be exercised. | ||
| cases := map[string]*feegrant.PeriodicAllowance{ | ||
| "basic spend limit over cap": { | ||
| Basic: feegrant.BasicAllowance{SpendLimit: over}, | ||
| PeriodSpendLimit: atCap, | ||
| PeriodCanSpend: atCap, | ||
| Period: time.Hour, | ||
| }, | ||
| "period spend limit over cap": { | ||
| Basic: feegrant.BasicAllowance{SpendLimit: atCap}, | ||
| PeriodSpendLimit: over, | ||
| PeriodCanSpend: atCap, | ||
| Period: time.Hour, | ||
| }, | ||
| "period can spend over cap": { | ||
| Basic: feegrant.BasicAllowance{SpendLimit: atCap}, | ||
| PeriodSpendLimit: atCap, | ||
| PeriodCanSpend: over, | ||
| Period: time.Hour, | ||
| }, | ||
| } | ||
|
|
||
| for name, allowance := range cases { | ||
| t.Run(name, func(t *testing.T) { | ||
| err := allowance.ValidateBasic() | ||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, feegrant.ErrTooManyDenoms) | ||
| }) | ||
| } | ||
| } | ||
|
claude[bot] marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.