-
Notifications
You must be signed in to change notification settings - Fork 117
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
[CORE-824] x/ratelimit
: Implement UpdateCapacityEndBlocker
#941
Changes from 3 commits
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 |
---|---|---|
|
@@ -9,17 +9,22 @@ import ( | |
"cosmossdk.io/store/prefix" | ||
storetypes "cosmossdk.io/store/types" | ||
"github.com/cosmos/cosmos-sdk/codec" | ||
"github.com/cosmos/cosmos-sdk/telemetry" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/dydxprotocol/v4-chain/protocol/dtypes" | ||
"github.com/dydxprotocol/v4-chain/protocol/lib" | ||
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics" | ||
"github.com/dydxprotocol/v4-chain/protocol/x/ratelimit/types" | ||
ratelimitutil "github.com/dydxprotocol/v4-chain/protocol/x/ratelimit/util" | ||
gometrics "github.com/hashicorp/go-metrics" | ||
) | ||
|
||
type ( | ||
Keeper struct { | ||
cdc codec.BinaryCodec | ||
storeKey storetypes.StoreKey | ||
bankKeeper types.BankKeeper | ||
cdc codec.BinaryCodec | ||
storeKey storetypes.StoreKey | ||
bankKeeper types.BankKeeper | ||
blockTimeKeeper types.BlockTimeKeeper | ||
|
||
// TODO(CORE-824): Implement `x/ratelimit` keeper | ||
|
||
|
@@ -32,13 +37,15 @@ func NewKeeper( | |
cdc codec.BinaryCodec, | ||
storeKey storetypes.StoreKey, | ||
bankKeeper types.BankKeeper, | ||
blockTimeKeeper types.BlockTimeKeeper, | ||
authorities []string, | ||
) *Keeper { | ||
return &Keeper{ | ||
cdc: cdc, | ||
storeKey: storeKey, | ||
bankKeeper: bankKeeper, | ||
authorities: lib.UniqueSliceToSet(authorities), | ||
cdc: cdc, | ||
storeKey: storeKey, | ||
bankKeeper: bankKeeper, | ||
blockTimeKeeper: blockTimeKeeper, | ||
authorities: lib.UniqueSliceToSet(authorities), | ||
} | ||
} | ||
|
||
|
@@ -116,28 +123,6 @@ func (k Keeper) ProcessDeposit( | |
}) | ||
} | ||
|
||
// GetBaseline returns the current capacity baseline for the given limiter. | ||
// `baseline` formula: | ||
// | ||
// baseline = max(baseline_minimum, baseline_tvl_ppm * current_tvl) | ||
func (k Keeper) GetBaseline( | ||
ctx sdk.Context, | ||
denom string, | ||
limiter types.Limiter, | ||
) *big.Int { | ||
// Get the current TVL. | ||
supply := k.bankKeeper.GetSupply(ctx, denom) | ||
currentTVL := supply.Amount.BigInt() | ||
|
||
return lib.BigMax( | ||
limiter.BaselineMinimum.BigInt(), | ||
lib.BigIntMulPpm( | ||
currentTVL, | ||
limiter.BaselineTvlPpm, | ||
), | ||
) | ||
} | ||
|
||
// SetLimitParams sets `LimitParams` for the given denom. | ||
// Also overwrites the existing `DenomCapacity` object for the denom with a default `capacity_list` of the | ||
// same length as the `limiters` list. Each `capacity` is initialized to the current baseline. | ||
|
@@ -161,11 +146,12 @@ func (k Keeper) SetLimitParams( | |
return | ||
} | ||
|
||
currentTvl := k.bankKeeper.GetSupply(ctx, limitParams.Denom) | ||
// Initialize the capacity list with the current baseline. | ||
newCapacityList := make([]dtypes.SerializableInt, len(limitParams.Limiters)) | ||
for i, limiter := range limitParams.Limiters { | ||
newCapacityList[i] = dtypes.NewIntFromBigInt( | ||
k.GetBaseline(ctx, limitParams.Denom, limiter), | ||
ratelimitutil.GetBaseline(currentTvl.Amount.BigInt(), limiter), | ||
) | ||
} | ||
// Set correspondong `DenomCapacity` in state. | ||
|
@@ -216,6 +202,85 @@ func (k Keeper) SetDenomCapacity( | |
b := k.cdc.MustMarshal(&denomCapacity) | ||
store.Set(key, b) | ||
} | ||
|
||
// Emit telemetry for the new capacity list. | ||
for i, capacity := range denomCapacity.CapacityList { | ||
telemetry.SetGaugeWithLabels( | ||
[]string{types.ModuleName, metrics.Capacity}, | ||
metrics.GetMetricValueFromBigInt(capacity.BigInt()), | ||
[]gometrics.Label{ | ||
metrics.GetLabelForStringValue(metrics.RateLimitDenom, denomCapacity.Denom), | ||
metrics.GetLabelForIntValue(metrics.LimiterIndex, i), | ||
}, | ||
) | ||
} | ||
} | ||
|
||
// UpdateAllCapacitiesEndBlocker is called during the EndBlocker to update the capacity for all limit params. | ||
func (k Keeper) UpdateAllCapacitiesEndBlocker( | ||
ctx sdk.Context, | ||
) { | ||
// Iterate through all the limit params in state. | ||
store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.LimitParamsKeyPrefix)) | ||
iterator := storetypes.KVStorePrefixIterator(store, []byte{}) | ||
|
||
defer iterator.Close() | ||
|
||
for ; iterator.Valid(); iterator.Next() { | ||
var limitParams types.LimitParams | ||
k.cdc.MustUnmarshal(iterator.Value(), &limitParams) | ||
k.updateCapacityForLimitParams(ctx, limitParams) | ||
} | ||
} | ||
|
||
// updateCapacityForLimitParams calculates current baseline for a denom and recovers some amount of capacity | ||
// towards baseline. | ||
// Assumes that the `LimitParams` exist in state. | ||
func (k Keeper) updateCapacityForLimitParams( | ||
ctx sdk.Context, | ||
limitParams types.LimitParams, | ||
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. For safety, should we validate 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. In general, I'm not a fan of passing values from function A to function B and then doing validations in function B. Instead the better pattern is to clearly document assumptions. Similarly, We actually do not assume 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. Update: checking this under in |
||
) { | ||
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. nit: this is a 100+ line logic method. Is there room to break this into smaller helper methods? For example:
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. Agree this function is too large. Broke down according to @BrendanChou 's suggestion below! |
||
tvl := k.bankKeeper.GetSupply(ctx, limitParams.Denom) | ||
|
||
capacityList := k.GetDenomCapacity(ctx, limitParams.Denom).CapacityList | ||
if len(capacityList) != len(limitParams.Limiters) { | ||
// This violates an invariant. Since this is in the `EndBlocker`, we log an error instead of panicking. | ||
k.Logger(ctx).Error( | ||
fmt.Sprintf( | ||
"denom (%s) capacity list length (%v) != limiters length (%v); skipping capacity update", | ||
limitParams.Denom, | ||
len(capacityList), | ||
len(limitParams.Limiters), | ||
), | ||
) | ||
return | ||
} | ||
|
||
timeSinceLastBlock := k.blockTimeKeeper.GetTimeSinceLastBlock(ctx) | ||
|
||
if timeSinceLastBlock < 0 { | ||
// This violates an invariant (current block time > prev block time). | ||
// Since this is in the `EndBlocker`, we log an error instead of panicking. | ||
k.Logger(ctx).Error( | ||
fmt.Sprintf( | ||
"timeSinceLastBlock (%v) <= 0; skipping capacity update", | ||
timeSinceLastBlock, | ||
), | ||
) | ||
return | ||
} | ||
|
||
newCapacityList := ratelimitutil.CalculateNewCapacityList( | ||
tvl.Amount.BigInt(), | ||
limitParams, | ||
capacityList, | ||
timeSinceLastBlock, | ||
) | ||
|
||
k.SetDenomCapacity(ctx, types.DenomCapacity{ | ||
Denom: limitParams.Denom, | ||
CapacityList: newCapacityList, | ||
}) | ||
} | ||
|
||
// GetDenomCapacity returns `DenomCapacity` for the given denom. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block is moved from below to before the
app.RateLimitKeeper
initialization.