fix x/stats GetStakedAmount and rename to GetStakedBaseTokens #3238
Conversation
WalkthroughThis PR refactors staking metrics across the protocol by renaming Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant StatsKeeper as Stats Keeper
participant StakingKeeper as Staking Keeper
participant Cache as Storage Cache
Caller->>StatsKeeper: GetStakedBaseTokens(ctx, delegatorAddr)
alt Cache Hit (within 900s)
StatsKeeper->>Cache: Retrieve CachedStakedBaseTokens
Cache-->>StatsKeeper: Return cached value
StatsKeeper-->>Caller: Return StakedBaseTokens
else Cache Miss or Expired
StatsKeeper->>StakingKeeper: GetDelegations(ctx, delegatorAddr)
StakingKeeper-->>StatsKeeper: Return delegations
loop For each delegation
StatsKeeper->>StakingKeeper: GetValidator(ctx, validatorAddr)
StakingKeeper-->>StatsKeeper: Return validator (Tokens, DelegatorShares)
Note over StatsKeeper: Calculate base tokens:<br/>userTokens = delegation.Shares × validator.Tokens / validator.DelegatorShares
StatsKeeper->>StatsKeeper: Accumulate in total
end
StatsKeeper->>Cache: Store CachedStakedBaseTokens with StakedBaseTokens
StatsKeeper-->>Caller: Return StakedBaseTokens
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protocol/x/stats/keeper/keeper.go (1)
314-322: Remove duplicate negative check.Lines 314-316 and lines 320-322 perform identical checks for
cachedStakedBaseTokens.CachedAt < 0. The second check is redundant.Apply this diff to remove the duplicate:
if cachedStakedBaseTokens.CachedAt < 0 { panic("cachedStakedBaseTokens.CachedAt is negative") } if ctx.BlockTime().Unix() < 0 { panic("Invariant violation: ctx.BlockTime().Unix() is negative") } -if cachedStakedBaseTokens.CachedAt < 0 { - panic("Invariant violation: cachedStakedBaseTokens.CachedAt is negative") -} if cachedStakedBaseTokens.CachedAt > ctx.BlockTime().Unix() { panic("Invariant violation: cachedStakedBaseTokens.CachedAt is greater than blocktime") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
protocol/x/stats/types/stats.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts(4 hunks)proto/dydxprotocol/stats/stats.proto(1 hunks)protocol/lib/metrics/metric_keys.go(2 hunks)protocol/x/affiliates/keeper/grpc_query.go(1 hunks)protocol/x/affiliates/keeper/keeper_test.go(1 hunks)protocol/x/affiliates/types/expected_keepers.go(1 hunks)protocol/x/feetiers/keeper/grpc_query.go(1 hunks)protocol/x/feetiers/keeper/grpc_query_test.go(1 hunks)protocol/x/feetiers/keeper/keeper.go(1 hunks)protocol/x/feetiers/keeper/keeper_test.go(1 hunks)protocol/x/feetiers/types/expected_keepers.go(1 hunks)protocol/x/stats/keeper/keeper.go(2 hunks)protocol/x/stats/keeper/keeper_test.go(5 hunks)protocol/x/stats/types/constants.go(1 hunks)protocol/x/stats/types/expected_keepers.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/types/expected_keepers.goprotocol/x/feetiers/types/expected_keepers.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/feetiers/keeper/grpc_query.goprotocol/x/feetiers/keeper/grpc_query_test.goprotocol/x/feetiers/keeper/keeper_test.goprotocol/x/feetiers/keeper/keeper.goprotocol/x/stats/keeper/keeper.goprotocol/x/stats/types/expected_keepers.goprotocol/x/affiliates/keeper/grpc_query.goprotocol/x/stats/keeper/keeper_test.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/types/expected_keepers.goprotocol/x/feetiers/types/expected_keepers.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/feetiers/keeper/grpc_query.goprotocol/x/feetiers/keeper/grpc_query_test.goprotocol/x/feetiers/keeper/keeper_test.goprotocol/x/feetiers/keeper/keeper.goprotocol/x/stats/keeper/keeper.goprotocol/x/stats/types/expected_keepers.goprotocol/x/affiliates/keeper/grpc_query.goprotocol/x/stats/keeper/keeper_test.go
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/types/expected_keepers.goprotocol/x/feetiers/types/expected_keepers.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/feetiers/keeper/grpc_query.goprotocol/x/feetiers/keeper/grpc_query_test.goprotocol/x/feetiers/keeper/keeper_test.goprotocol/x/feetiers/keeper/keeper.goprotocol/x/stats/keeper/keeper.goprotocol/x/stats/types/expected_keepers.goprotocol/x/affiliates/keeper/grpc_query.goprotocol/x/stats/keeper/keeper_test.go
🧬 Code graph analysis (6)
protocol/x/affiliates/keeper/keeper_test.go (1)
protocol/x/stats/types/constants.go (1)
StakedBaseTokensCacheDurationSeconds(4-4)
protocol/x/feetiers/keeper/grpc_query_test.go (2)
protocol/x/stats/types/stats.pb.go (3)
CachedStakedBaseTokens(429-435)CachedStakedBaseTokens(439-439)CachedStakedBaseTokens(440-442)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)
protocol/x/feetiers/keeper/keeper_test.go (3)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
CachedStakedBaseTokens(158-167)CachedStakedBaseTokens(583-629)protocol/x/stats/types/stats.pb.go (3)
CachedStakedBaseTokens(429-435)CachedStakedBaseTokens(439-439)CachedStakedBaseTokens(440-442)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (1)
protocol/x/stats/types/stats.pb.go (3)
CachedStakedBaseTokens(429-435)CachedStakedBaseTokens(439-439)CachedStakedBaseTokens(440-442)
protocol/x/stats/keeper/keeper.go (7)
protocol/x/stats/types/keys.go (1)
CachedStakeAmountKeyPrefix(36-36)indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
CachedStakedBaseTokens(158-167)CachedStakedBaseTokens(583-629)protocol/x/stats/types/stats.pb.go (3)
CachedStakedBaseTokens(429-435)CachedStakedBaseTokens(439-439)CachedStakedBaseTokens(440-442)protocol/x/stats/types/constants.go (1)
StakedBaseTokensCacheDurationSeconds(4-4)protocol/lib/metrics/lib.go (1)
IncrCounterWithLabels(20-22)protocol/lib/metrics/metric_keys.go (4)
StatsGetStakedBaseTokensCacheHit(30-30)StatsGetStakedBaseTokensLatencyCacheHit(41-41)StatsGetStakedBaseTokensCacheMiss(31-31)StatsGetStakedBaseTokensLatencyCacheMiss(42-42)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)
protocol/x/stats/keeper/keeper_test.go (5)
protocol/lib/big_math.go (2)
BigPow10(36-51)BigU(10-12)protocol/lib/constants.go (1)
BaseDenomExponent(22-22)protocol/testutil/constants/addresses.go (2)
AliceValAddress(14-14)AliceAccAddress(10-10)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)protocol/x/stats/types/stats.pb.go (3)
CachedStakedBaseTokens(429-435)CachedStakedBaseTokens(439-439)CachedStakedBaseTokens(440-442)
⏰ 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). (39)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: protocol-gen
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: check-build-bazooka
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: test / run_command
- GitHub Check: golangci-lint
- GitHub Check: test-coverage-upload
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-testnet
- GitHub Check: liveness-test
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-race
- GitHub Check: lint
- GitHub Check: container-tests
- GitHub Check: build-and-push-mainnet
- GitHub Check: benchmark
- GitHub Check: run_command
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (21)
protocol/x/affiliates/keeper/grpc_query.go (1)
43-43: LGTM! Method rename is consistent.The update from
GetStakedAmounttoGetStakedBaseTokensaligns with the PR objectives to correctly represent staked base tokens rather than raw delegator shares.protocol/x/affiliates/types/expected_keepers.go (1)
11-11: LGTM! Interface method renamed consistently.The rename from
GetStakedAmounttoGetStakedBaseTokensin the StatsKeeper interface clarifies that this method returns base tokens rather than raw delegator shares.protocol/x/feetiers/keeper/grpc_query.go (1)
155-155: LGTM! Method rename is consistent.The update to
GetStakedBaseTokenscorrectly aligns with the new base token semantics used for staking discount calculations.protocol/x/affiliates/keeper/keeper_test.go (1)
223-223: LGTM! Cache duration constant renamed consistently.The update from
StakedAmountCacheDurationSecondstoStakedBaseTokensCacheDurationSecondsaligns with the broader rename to base token semantics.protocol/x/stats/types/expected_keepers.go (1)
19-19: LGTM! Essential addition for shares-to-tokens conversion.The
GetValidatormethod is necessary to retrieve validator information for converting delegator shares to base tokens, which is the core fix mentioned in the PR objectives.protocol/x/feetiers/keeper/keeper.go (1)
162-162: LGTM! Method rename is consistent.The update to
GetStakedBaseTokenscorrectly obtains base token amounts for staking discount calculations in the perpetual fee flow.protocol/x/feetiers/keeper/keeper_test.go (1)
507-510: LGTM! Test fixture updates are consistent.The updates to
UnsafeSetCachedStakedBaseTokens,CachedStakedBaseTokens, andStakedBaseTokensare all consistent with the base token semantics. The test correctly sets up staked base tokens for discount validation.protocol/x/feetiers/types/expected_keepers.go (1)
16-16: LGTM! Interface method renamed consistently.The rename from
GetStakedAmounttoGetStakedBaseTokensin the StatsKeeper interface clarifies that this method returns base tokens, aligning with the PR's objective to correctly convert delegator shares to tokens.protocol/x/stats/types/constants.go (1)
3-4: LGTM! Constant rename aligns with base tokens terminology.The cache duration value remains 900 seconds as expected, with only the naming updated to reflect the new base-tokens semantics.
protocol/lib/metrics/metric_keys.go (2)
30-31: LGTM! Metrics keys updated to reflect base tokens terminology.The cache hit/miss metrics are correctly renamed to align with the new
GetStakedBaseTokensAPI.
41-42: LGTM! Latency metrics keys updated consistently.The latency metrics for cache hit/miss paths are correctly renamed to match the base tokens terminology.
proto/dydxprotocol/stats/stats.proto (1)
78-89: LGTM! Protobuf changes are well-structured and backward compatible.The message and field renames align with the new base tokens semantics. The new
cached_atfield is properly positioned as field 2, and the preserved field number forstaked_base_tokensensures backward compatibility with existing cached data (which will simply expire on first read).protocol/x/feetiers/keeper/grpc_query_test.go (1)
543-546: LGTM! Test correctly updated to use new base tokens API.The test properly uses
UnsafeSetCachedStakedBaseTokenswith the newCachedStakedBaseTokenstype andStakedBaseTokensfield, maintaining consistency with the API changes.indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (1)
156-179: LGTM! Generated TypeScript code correctly reflects protobuf changes.The interfaces, SDK types, and serialization logic are properly updated to match the protobuf definitions, with consistent naming for
CachedStakedBaseTokens,stakedBaseTokens, andcachedAtfields.Also applies to: 576-629
protocol/x/stats/keeper/keeper.go (4)
297-302: LGTM! Method renamed with updated documentation.The rename from
GetStakedAmounttoGetStakedBaseTokensclearly conveys that this returns actual base tokens (converted from shares) rather than raw staking shares.
353-369: Excellent fix! Proper share-to-token conversion implemented.This is the core fix mentioned in the PR description. The implementation correctly:
- Fetches each validator from the staking keeper
- Converts delegation shares to base tokens using
validator.TokensFromShares()- Uses the validator's exchange rate to account for slashing, rewards, etc.
- Gracefully handles invalid/missing validators by skipping them
The use of
RoundInt()on Line 368 is appropriate - truncating to integer tokens is the conservative approach for security-sensitive calculations like fee discounts.
372-384: LGTM! Cache update logic correctly uses new types.The cache is properly updated with
CachedStakedBaseTokenscontaining both the calculatedStakedBaseTokensand the currentCachedAttimestamp.
386-390: LGTM! Unsafe setter renamed and updated.The method signature is correctly updated to accept
*types.CachedStakedBaseTokens, maintaining consistency with the public API changes.protocol/x/stats/keeper/keeper_test.go (3)
300-373: Excellent test coverage for share-to-token conversion!The test cases comprehensively validate the new conversion logic across different validator exchange rates:
- 1:1 (no slashing/rewards)
- 1.5:1 and 2:1 (validator gained value)
- 0.5:1 (validator lost value, e.g., from slashing)
The test properly sets up validator state with realistic
TokensandDelegatorShares, then verifies thatGetStakedBaseTokensreturns the correctly calculated base tokens using the formula:userShares × validatorTokens ÷ validatorShares.
375-391: LGTM! Cache hit test updated correctly.The test properly uses
UnsafeSetCachedStakedBaseTokenswith the newCachedStakedBaseTokenstype and verifies thatGetStakedBaseTokensreturns the cached value.
393-427: LGTM! Cache miss test validates expiration and recalculation.The test correctly verifies that expired cached data (beyond the 900-second duration) triggers recalculation from the staking keeper, returning the updated delegation amount.
|
@Mergifyio backport release/protocol/v9.x |
✅ Backports have been created
|
(cherry picked from commit 83d1e18)
#3238) (#3239) Co-authored-by: Tian <[email protected]>
Changelist
Need to convert delegator shares to tokens to get staked tokens
Test Plan
updated unit test
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit