[OTE-693][OTE-691] Add keeper methods for affiliates #2141
Conversation
WalkthroughThe changes enhance the staking and affiliate management capabilities within the protocol. A new message for caching staked amounts has been introduced, along with expanded functionalities in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StatsKeeper
participant StakingKeeper
User->>StatsKeeper: Request Staked Amount
StatsKeeper->>StakingKeeper: Check Cache
alt Cache Hit
StatsKeeper-->>User: Return Cached Staked Amount
else Cache Miss
StatsKeeper->>StakingKeeper: Fetch Delegations
StakingKeeper-->>StatsKeeper: Return Delegations
StatsKeeper->>StatsKeeper: Calculate Total Staked Amount
StatsKeeper-->>User: Return Staked Amount
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
affanv14
changed the title
![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
Outside diff range, codebase verification and nitpick comments (4)
protocol/x/affiliates/types/expected_keepers.go (1)
9-11: Add documentation forStatsKeeperinterface.Consider adding comments to describe the purpose and usage of the
StatsKeeperinterface.// StatsKeeper defines the expected interface for retrieving staking information. type StatsKeeper interface {protocol/x/stats/types/expected_keepers.go (2)
12-14: Add documentation forEpochsKeeperinterface.Consider adding comments to describe the purpose and usage of the
EpochsKeeperinterface.// EpochsKeeper defines the expected interface for retrieving epoch information. type EpochsKeeper interface {
16-18: Add documentation forStakingKeeperinterface.Consider adding comments to describe the purpose and usage of the
StakingKeeperinterface.// StakingKeeper defines the expected interface for managing staking operations. type StakingKeeper interface {protocol/x/stats/keeper/keeper.go (1)
284-353: Review error handling strategy inGetStakedAmountfunction.The function currently uses
panicfor error handling, which may not align with the broader error management strategy observed in other parts of the codebase where errors are wrapped and returned. Consider reviewing and potentially refactoring the error handling to ensure consistency and robustness.
sdk.AccAddressFromBech32andk.stakingKeeper.GetDelegatorDelegationsare handled withpanicinprotocol/x/stats/keeper/keeper.go.Analysis chain
GetStakedAmountfunction is efficiently implemented with caching and telemetry.The function effectively retrieves the staked amount using caching to enhance performance and integrates telemetry metrics for monitoring.
Ensure that error handling for
sdk.AccAddressFromBech32andk.stakingKeeper.GetDelegatorDelegationsaligns with the application's error management strategy.
Run the following script to verify the error handling:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling for `sdk.AccAddressFromBech32` and `k.stakingKeeper.GetDelegatorDelegations`. # Test: Search for error handling in the codebase. Expect: Proper error management strategy. rg --type go -A 5 $'AccAddressFromBech32' | rg 'panic\|return' rg --type go -A 5 $'GetDelegatorDelegations' | rg 'panic\|return'Length of output: 126
Script:
#!/bin/bash # Description: Search for occurrences of `sdk.AccAddressFromBech32` and `k.stakingKeeper.GetDelegatorDelegations` and inspect surrounding context for error handling. # Search for `sdk.AccAddressFromBech32` and display surrounding lines for context rg --type go -A 10 -B 2 'sdk\.AccAddressFromBech32' # Search for `k.stakingKeeper.GetDelegatorDelegations` and display surrounding lines for context rg --type go -A 10 -B 2 'k\.stakingKeeper\.GetDelegatorDelegations'Length of output: 48808
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/stats/types/stats.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- proto/dydxprotocol/stats/stats.proto (1 hunks)
- protocol/app/app.go (2 hunks)
- protocol/lib/metrics/metric_keys.go (2 hunks)
- protocol/testutil/keeper/auth.go (2 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/rewards.go (1 hunks)
- protocol/testutil/keeper/staking.go (1 hunks)
- protocol/testutil/keeper/stats.go (3 hunks)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (2 hunks)
- protocol/x/affiliates/types/expected_keepers.go (1 hunks)
- protocol/x/affiliates/types/keys.go (1 hunks)
- protocol/x/stats/keeper/keeper.go (5 hunks)
- protocol/x/stats/keeper/keeper_test.go (2 hunks)
- protocol/x/stats/types/constants.go (1 hunks)
- protocol/x/stats/types/expected_keepers.go (1 hunks)
- protocol/x/stats/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/stats/types/constants.go
Additional comments not posted (34)
protocol/x/affiliates/types/keys.go (1)
14-18: LGTM! Constants are well-defined.The new constants for managing affiliate relationships are clear and appropriately named.
protocol/x/stats/types/keys.go (1)
35-36: Addition ofCachedStakeAmountKeyPrefixis appropriate.The new constant
CachedStakeAmountKeyPrefixaligns with the existing naming conventions and structure, enhancing the caching functionality.protocol/testutil/keeper/staking.go (1)
1-44: New filestaking.goand functioncreateStakingKeeperare well-implemented.The function
createStakingKeepercorrectly initializes the staking keeper with the required dependencies, and the imports are appropriate for the functionality.protocol/testutil/keeper/stats.go (1)
Line range hint
11-22: Modification ofcreateStatsKeeperwithstakingKeeperis appropriate.The addition of the
stakingKeeperparameter enhances integration with the staking module. Ensure that all function calls are updated to match the new signature.proto/dydxprotocol/stats/stats.proto (1)
65-76: LGTM! New messageCachedStakeAmountis well-defined.The addition of the
CachedStakeAmountmessage enhances the protocol's ability to manage and retrieve staking data efficiently. The use of a custom type forstaked_amountand a timestamp forcached_atis appropriate.protocol/testutil/keeper/auth.go (1)
45-46: LGTM! Permissions for staking pools are appropriately added.The addition of
BondedPoolNameandNotBondedPoolNamewithBurnerandStakingroles enhances the account keeper's functionality for managing staking-related permissions.protocol/testutil/keeper/rewards.go (1)
59-85: LGTM! New keeper initializations enhance functionality.The addition of
accountsKeeper,bankKeeper, andstakingKeeperexpands the functionality of theRewardsKeepersfunction, allowing for more comprehensive management of rewards.protocol/testutil/keeper/listing.go (4)
80-84: Ensure correct initialization ofaccountsKeeper.The
accountsKeeperis initialized correctly with the necessary parameters. Verify that thecreateAccountKeeperfunction handles these parameters appropriately.Run the following script to verify the function implementation:
Verification successful
Verified: Correct initialization of
accountsKeeper.The
createAccountKeeperfunction is defined with the expected parameters and return types, confirming that the initialization ofaccountsKeeperin theListingKeepersfunction is handled correctly.
- Location of
createAccountKeeper:protocol/testutil/keeper/auth.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `createAccountKeeper`. # Test: Search for the function implementation. Expect: Correct handling of parameters. ast-grep --lang go --pattern $'func createAccountKeeper($_, $_, $_, $_) ($_)'Length of output: 78
Script:
#!/bin/bash # Use ripgrep to search for the definition of the `createAccountKeeper` function in the codebase. rg 'func createAccountKeeper' --context 5Length of output: 830
86-91: Ensure correct initialization ofbankKeeper.The
bankKeeperis initialized withaccountsKeeper. Verify that thecreateBankKeeperfunction handles these parameters appropriately.Run the following script to verify the function implementation:
Verification successful
Initialization of
bankKeeperis correct.The
createBankKeeperfunction appropriately handles theaccountKeeperparameter, ensuring correct initialization ofbankKeeper.
- The
accountKeeperis passed directly toNewBaseKeeper, which is consistent with expected usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `createBankKeeper`. # Test: Search for the function implementation. Expect: Correct handling of parameters. ast-grep --lang go --pattern $'func createBankKeeper($_, $_, $_, $_, $_) ($_)'Length of output: 79
Script:
#!/bin/bash # Description: Search for the definition of `createBankKeeper` in the codebase. # Use ripgrep to find the function definition. rg 'func createBankKeeper' -A 5Length of output: 423
Script:
#!/bin/bash # Description: Extract the full implementation of `createBankKeeper` to verify parameter handling. # Use ripgrep to extract the full function implementation. rg 'func createBankKeeper' -A 20 protocol/testutil/keeper/bank.goLength of output: 577
93-99: Ensure correct initialization ofstakingKeeper.The
stakingKeeperis initialized withaccountsKeeperandbankKeeper. Verify that thecreateStakingKeeperfunction handles these parameters appropriately.Run the following script to verify the function implementation:
Verification successful
Initialization of
stakingKeeperis correct.The
createStakingKeeperfunction correctly handles theaccountKeeperandbankKeeperparameters by passing them to thekeeper.NewKeeperfunction, ensuring proper initialization ofstakingKeeper.
- Verified in
protocol/testutil/keeper/staking.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `createStakingKeeper`. # Test: Search for the function implementation. Expect: Correct handling of parameters. ast-grep --lang go --pattern $'func createStakingKeeper($_, $_, $_, $_, $_, $_) ($_)'Length of output: 86
Script:
#!/bin/bash # Description: Search for any function named `createStakingKeeper` in the codebase. # Test: Search for the function definition by name only. rg 'func createStakingKeeper' -A 10 --no-headingLength of output: 732
Script:
#!/bin/bash # Description: Extract the full implementation of `createStakingKeeper` to verify parameter handling. # Test: Extract the function implementation to analyze parameter usage. rg 'func createStakingKeeper' -A 50 --no-heading protocol/testutil/keeper/staking.goLength of output: 767
107-107: EnsurestatsKeeperintegration withstakingKeeper.The
statsKeepernow includesstakingKeeper. Verify that thecreateStatsKeeperfunction handles this integration correctly.Run the following script to verify the function implementation:
Verification successful
Integration of
stakingKeeperincreateStatsKeeperis correct.The
createStatsKeeperfunction inprotocol/testutil/keeper/stats.gocorrectly integratesstakingKeeperby passing it to thekeeper.NewKeeperfunction. This confirms that the integration is handled as expected.
- Location:
protocol/testutil/keeper/stats.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `createStatsKeeper`. # Test: Search for the function implementation. Expect: Correct integration with `stakingKeeper`. ast-grep --lang go --pattern $'func createStatsKeeper($_, $_, $_, $_, $_) ($_)'Length of output: 80
Script:
#!/bin/bash # Search for any function named `createStatsKeeper` in the repository to verify its existence and parameters. rg 'func createStatsKeeper' -A 10 --glob '*.go'Length of output: 847
Script:
#!/bin/bash # Extract the complete implementation of the `createStatsKeeper` function to verify the integration of `stakingKeeper`. ast-grep --lang go --pattern 'func createStatsKeeper($_, $_, $_, $_, $_) ($_) { $$$ }'Length of output: 88
Script:
#!/bin/bash # Use `rg` to extract the complete implementation of the `createStatsKeeper` function, capturing more lines to ensure the full function body is included. rg 'func createStatsKeeper' -A 50 --glob '*.go'Length of output: 1942
protocol/lib/metrics/metric_keys.go (4)
30-30: LGTM! Addition ofStatsGetStakedAmountCacheHit.The constant is consistent with the existing metrics framework.
31-31: LGTM! Addition ofStatsGetStakedAmountCacheMiss.The constant is consistent with the existing metrics framework.
41-41: LGTM! Addition ofStatsGetStakedAmountLatencyCacheHit.The constant is consistent with the existing metrics framework.
42-42: LGTM! Addition ofStatsGetStakedAmountLatencyCacheMiss.The constant is consistent with the existing metrics framework.
protocol/x/affiliates/keeper/keeper.go (8)
24-24: LGTM! Addition ofstatsKeepertoKeeperstruct.The addition is consistent with the PR objectives and enhances the functionality of the
Keeper.
32-38: LGTM! Inclusion ofstatsKeeperinNewKeeper.The inclusion is consistent with the PR objectives and enhances the functionality of the
Keeper.
53-64: LGTM! Implementation ofRegisterAffiliate.The function logic is sound and consistent with the intended functionality.
66-72: LGTM! Implementation ofGetReferredBy.The function logic is sound and consistent with the intended functionality.
108-122: LGTM! Implementation ofGetAllAffiliateTiers.The function logic is sound and consistent with the intended functionality.
124-142: LGTM! Implementation ofGetTakerFeeShare.The function logic is sound and consistent with the intended functionality.
144-184: LGTM! Implementation ofGetTierForAffiliate.The function logic is sound and consistent with the intended functionality.
186-195: LGTM! Implementation ofUpdateAffiliateTiers.The function logic is sound and consistent with the intended functionality.
protocol/x/affiliates/keeper/keeper_test.go (6)
Line range hint
16-22: LGTM!The
TestLoggerfunction correctly verifies that the logger is initialized.
24-62: LGTM!The
TestRegisterAffiliatefunction effectively tests both successful and duplicate affiliate registration scenarios.
64-86: LGTM!The
TestAddReferredVolumefunction accurately tests the addition and updating of referred volumes.
88-134: LGTM!The
TestGetTakerFeeShareViaReferredVolumefunction effectively tests fee share calculations and tier upgrades based on referred volume.
136-179: LGTM!The
TestGetTakerFeeShareViaStakedAmountfunction accurately tests fee share calculations and tier upgrades based on staked amounts.
181-210: LGTM!The
TestUpdateAffiliateTiersfunction effectively tests tier updates and validation for proper sorting.protocol/x/stats/keeper/keeper_test.go (1)
297-308: LGTM!The
TestGetStakedAmountfunction correctly tests the retrieval of staked amounts, ensuring accurate delegation setup and verification.protocol/testutil/keeper/clob.go (1)
116-141: LGTM!The added initializations for
accountsKeeper,bankKeeper, andstakingKeeperenhance the test context, improving the realism and coverage of the testing framework.protocol/x/stats/keeper/keeper.go (2)
28-28: Addition ofstakingKeeperfield inKeeperstruct is appropriate.The inclusion of
stakingKeeperenables interaction with the staking module, aligning with the PR objectives.
38-46: Enhancement ofNewKeeperfunction withstakingKeeperparameter is well-implemented.The addition of
stakingKeeperas a parameter supports dependency injection, facilitating interaction with the staking module.protocol/app/app.go (1)
Line range hint
1022-1206: Integration ofStakingKeeperandStatsKeeperinNewfunction enhances module capabilities.The inclusion of
app.StakingKeeperandapp.StatsKeeperas parameters improves the functionality and integration of thestatsModuleandaffiliatesModule.
| func (k Keeper) GetReferredVolume(ctx sdk.Context, affiliateAddr string) (dtypes.SerializableInt, bool) { | ||
| affiliateReferredVolumePrefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.ReferredVolmeKeyPrefix)) | ||
| if !affiliateReferredVolumePrefixStore.Has([]byte(affiliateAddr)) { | ||
| return dtypes.NewInt(0), false | ||
| } | ||
| var referredVolume dtypes.SerializableInt | ||
| referredVolume.Unmarshal(affiliateReferredVolumePrefixStore.Get([]byte(affiliateAddr))) | ||
| return referredVolume, true | ||
| } |
There was a problem hiding this comment.
Fix typo in prefix key name.
There is a typo in the prefix key name ReferredVolmeKeyPrefix. It should be ReferredVolumeKeyPrefix.
Apply this diff to fix the typo:
affiliateReferredVolumePrefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.ReferredVolmeKeyPrefix))
+affiliateReferredVolumePrefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.ReferredVolumeKeyPrefix))Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2 hunks)
- proto/dydxprotocol/stats/stats.proto (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/staking.go (1 hunks)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (2 hunks)
- protocol/x/stats/keeper/keeper_test.go (2 hunks)
- protocol/x/stats/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- proto/dydxprotocol/stats/stats.proto
- protocol/testutil/keeper/clob.go
- protocol/testutil/keeper/staking.go
- protocol/x/affiliates/keeper/keeper.go
- protocol/x/stats/keeper/keeper_test.go
- protocol/x/stats/types/expected_keepers.go
Additional comments not posted (10)
protocol/x/affiliates/keeper/keeper_test.go (5)
24-62: Comprehensive test coverage for affiliate registration.The
TestRegisterAffiliatefunction effectively tests both successful registration and error handling for existing referees. The test cases are well-structured and cover the expected scenarios.
64-86: Effective testing of referred volume management.The
TestAddReferredVolumefunction accurately tests the addition and updating of referred volumes. The logic for tracking total volume is correctly validated.
88-135: Thorough validation of fee share calculation via referred volume.The
TestGetTakerFeeShareViaReferredVolumefunction effectively tests the calculation and updating of taker fee shares based on referred volume. The test cases are comprehensive and validate the logic accurately.
137-181: Comprehensive testing of fee share calculation via staked amount.The
TestGetTakerFeeShareViaStakedAmountfunction accurately tests the calculation and updating of taker fee shares based on staked amounts. The test cases are well-structured and validate the logic effectively.
183-212: Effective validation of affiliate tier updates.The
TestUpdateAffiliateTiersfunction thoroughly tests the updating of affiliate tiers, including proper validation and error handling for invalid configurations. The test cases are comprehensive and well-structured.indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (5)
120-129: Correct definition ofCachedStakeAmountinterface.The
CachedStakeAmountinterface is well-defined, with properties for staked amount and cached time. It aligns with the expected structure for managing staking data.
132-141: Consistent definition ofCachedStakeAmountSDKType.The
CachedStakeAmountSDKTypeis correctly defined, mirroring theCachedStakeAmountinterface with snake_case properties. This consistency ensures seamless integration with the SDK.
508-513: Correct initialization increateBaseCachedStakeAmount.The
createBaseCachedStakeAmountfunction correctly initializes aCachedStakeAmountobject with default values, adhering to best practices for object creation.
515-552: Accurate encoding and decoding forCachedStakeAmount.The encoding and decoding methods for
CachedStakeAmountare correctly implemented, ensuring proper serialization and deserialization of the data structure.
554-559: Correct implementation offromPartialforCachedStakeAmount.The
fromPartialmethod is correctly implemented, allowing for flexible creation ofCachedStakeAmountobjects from partial inputs.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/types/keys.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/affiliates/keeper/keeper.go
Additional comments not posted (3)
protocol/x/affiliates/types/keys.go (3)
14-14: Addition ofReferredByKeyPrefixis appropriate.The constant
ReferredByKeyPrefixis well-named and serves its purpose for prefixing referral-related keys.
16-16: Addition ofReferredVolumeKeyPrefixis appropriate.The constant
ReferredVolumeKeyPrefixis well-named and serves its purpose for prefixing referral volume-related keys.
18-18: Addition ofAffiliateTiersKeyis appropriate.The constant
AffiliateTiersKeyis well-named and serves its purpose for keys related to affiliate tiers.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/indexer/events/events.pb.gois excluded by!**/*.pb.go
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (6 hunks)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
Additional comments not posted (3)
indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (3)
3755-3756: LGTM:fromPartialmethod updatedThe
fromPartialmethod forRegisterAffiliateEventV1has been updated to remove the initialization of theregisteredAtBlockproperty, consistent with its removal from the interface.
3755-3756: LGTM:encodemethod updatedThe
encodemethod forRegisterAffiliateEventV1has been updated to remove the encoding of theregisteredAtBlockproperty, consistent with its removal from the interface.
3755-3756: LGTM:decodemethod updatedThe
decodemethod forRegisterAffiliateEventV1has been updated to remove the decoding of theregisteredAtBlockproperty, consistent with its removal from the interface.
| affiliate: "" | ||
| }; |
There was a problem hiding this comment.
Interface property removed: registeredAtBlock
The registeredAtBlock property has been removed from the RegisterAffiliateEventV1 interface. This property likely stored the block number when the affiliate was registered.
Consider the implications of this removal:
- Is the block number information no longer needed for affiliate registration events?
- Will this information be tracked in a different way or is it no longer relevant?
Ensure that removing this property doesn't adversely affect other parts of the system that may have depended on this information.
Corresponding change in RegisterAffiliateEventV1SDKType
The registered_at_block property has also been removed from the RegisterAffiliateEventV1SDKType interface, mirroring the change made to RegisterAffiliateEventV1.
See the previous comment for considerations around this change.
| } | ||
|
|
||
| for index, tier := range tiers { | ||
| if referredVolume.Cmp(lib.BigU(tier.ReqReferredVolumeQuoteQuantums)) >= 0 { |
There was a problem hiding this comment.
Break early in the for loop
for index, tier := range tiers {
if referredVolume.Cmp(lib.BigU(tier.ReqReferredVolumeQuoteQuantums)) < 0 {
break
}
// safe to do as tier cannot be negative
currentTier = uint32(index)
}
```
There was a problem hiding this comment.
Let's also add comment about assumptions on referredVolume, ReqStakedWholeCoins being strictly increasing in the tiers
There was a problem hiding this comment.
Did we end up adding comment for this? Would be helpful for both loops
There was a problem hiding this comment.
Missed this comment! fixing
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range, codebase verification and nitpick comments (1)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts (1)
Line range hint
87-142: Encoding and decoding logic updates approved; suggest adding tests.The updates to the encoding and decoding logic in
AffiliateTiers_Tiercorrectly reflect the interface changes. To ensure these changes work as expected, consider adding unit tests specifically for these functions.Would you like me to help by generating unit test code for these functions?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/affiliates/types/affiliates.pb.gois excluded by!**/*.pb.go
Files selected for processing (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts (5 hunks)
- proto/dydxprotocol/affiliates/affiliates.proto (1 hunks)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (2 hunks)
- protocol/x/stats/keeper/keeper.go (5 hunks)
- protocol/x/stats/keeper/keeper_test.go (2 hunks)
Additional comments not posted (18)
proto/dydxprotocol/affiliates/affiliates.proto (1)
13-13: Field Renaming and Type Change:req_referred_volume_quote_quantumsThe field
req_referred_volumehas been renamed toreq_referred_volume_quote_quantumsand its type changed fromuint32touint64. This change likely reflects a need for handling larger volumes, which is appropriate given the context of financial transactions where large numbers can be common. The renaming also clarifies that the volume is measured in quote quantums, improving the readability and understanding of the data type.indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts (2)
19-19: Interface changes approved; verify impact on other modules.The removal of the
levelproperty and renaming ofreqReferredVolumetoreqReferredVolumeQuoteQuantumsare clear and seem justified. However, ensure that these changes do not adversely affect other parts of the application where this interface is used.
31-31: SDK Interface changes approved; verify impact on SDK consumers.The changes to the
AffiliateTiers_TierSDKTypeinterface are consistent with those in theAffiliateTiers_Tier. Ensure that SDK consumers are updated to accommodate these changes.Verification successful
Renaming consistently applied; verify impact on external SDK consumers.
The
req_referred_volumeproperty has been successfully renamed toreq_referred_volume_quote_quantumswithin the codebase. Ensure that any external SDK consumers are updated to accommodate this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of SDK interface changes on consumers. # Test: Search for the usage of removed and renamed SDK properties. Expect: No occurrences of the old property names. rg --type typescript -A 5 $'req_referred_volume'Length of output: 82
Script:
#!/bin/bash # Description: Verify the impact of SDK interface changes on consumers. # Test: Search for the usage of removed and renamed SDK properties. Expect: No occurrences of the old property names. rg --glob '*.ts' -A 5 'req_referred_volume'Length of output: 699
protocol/x/affiliates/keeper/keeper.go (8)
23-23: Addition ofstatsKeeperto theKeeperstruct.The addition of
statsKeeperto theKeeperstruct is a significant change that enhances the struct's functionality by integrating statistics-related capabilities. This change aligns with the PR's objectives to enhance staking and affiliate management capabilities.
31-37: Constructor modification to includestatsKeeper.The constructor for the
Keeperstruct has been modified to includestatsKeeperas a parameter. This change is necessary to initialize the new field added to theKeeperstruct. The code correctly initializesstatsKeeperand maintains the existing initialization logic for other fields.
53-71: New method:RegisterAffiliate.This method adds functionality to register an affiliate address for a referee. It includes checks for existing affiliations and validates the addresses' formats using
sdk.AccAddressFromBech32, which is a robust way to handle address validation. The method also correctly handles errors usingerrorsmod.Wrapf, providing clear error messages. However, the method lacks detailed comments explaining its purpose and the logic behind each step, which could improve maintainability and readability.This method has been previously reviewed, and similar comments have been made regarding the need for additional comments and error handling improvements.
73-80: New method:GetReferredBy.This method retrieves the affiliate address associated with a given referee. It uses a prefix store to check and retrieve the affiliate address, which is a standard approach in Cosmos SDK modules for handling key-value data. The method is concise and correctly handles the case where the referee is not found.
117-128: New method:GetReferredVolume.This method retrieves the total referred volume for a specific affiliate. It correctly handles the case where no volume is recorded for the affiliate, returning zero. The use of
big.Intfor handling large numbers is appropriate, and the method's error handling is consistent with the rest of the module.
130-145: New method:GetAllAffiliateTiers.This method fetches all tiers associated with affiliates, providing a mechanism to manage and query affiliate structures effectively. It handles the case where affiliate tiers are not initialized by returning an appropriate error. The method's implementation is straightforward and uses standard Cosmos SDK patterns for data retrieval and error handling.
147-166: New method:GetTakerFeeShare.This method enhances the tier management system by allowing retrieval of fee shares for a given address. It integrates the
GetReferredByandGetTierForAffiliatemethods to determine the fee share based on the affiliate's tier. The method handles errors appropriately and returns detailed information about the affiliate and the fee share.
217-224: Method modification:UpdateAffiliateTiers.This method has been updated to handle new requirements for checking strictly increasing volume and staking requirements. The implementation uses standard Cosmos SDK patterns for setting data in the store. The TODO comment on line 221 suggests further enhancements that should be addressed in future updates.
protocol/x/affiliates/keeper/keeper_test.go (4)
276-289: Ensure correct updating of affiliate tiers.The function
TestUpdateAffiliateTiersvalidates the updating of affiliate tiers, confirming that changes are correctly reflected in the system. This is essential for maintaining the integrity of tier data within the system. The test appears to be implemented correctly and effectively checks the updated values against expected results.
27-74: Consider restructuring tests for clarity and independence.The tests in
TestRegisterAffiliate_GetReferredByare designed to validate both registration and retrieval functionalities. However, the existing comments suggest making the tests independent for better scalability and clarity. Consider restructuring the tests to separate the registration and retrieval functionalities into distinct test cases. This approach will make it easier to manage and extend the tests in the future.Additionally, ensure that the test cases use valid dYdX bech32 addresses as suggested in the previous comments. This will ensure that the tests are realistic and adhere to expected input formats.
124-170: Assess fee share calculations based on referred volume.
TestGetTakerFeeShareViaReferredVolumetests the calculation of taker fee shares based on referred volumes, including tier upgrades. This is a critical functionality to ensure that affiliates are rewarded correctly based on their referred business. The test appears to cover the necessary scenarios, but consider adding a test case where the referred volume qualifies for a higher tier but the staked amount does not, as suggested in the previous comments. This will help verify that the tier calculation prioritizes the correct metrics under various conditions.
172-223: Validate fee share calculations based on staked amounts.The function
TestGetTakerFeeShareViaStakedAmounttests the calculation of taker fee shares based on staked amounts. It is important to ensure that the system accurately reflects tier upgrades based on staking thresholds. As suggested in the previous comments, adding a test case where the referred volume and staked amount qualify for different tiers would be beneficial. This will help confirm that the system correctly identifies and applies the highest qualifying tier.protocol/x/stats/keeper/keeper.go (3)
29-29: Addition ofstakingKeepertoKeeperstruct.The addition of the
stakingKeeperfield to theKeeperstruct is crucial for the new functionalities related to staking. This change aligns with the PR's objectives to enhance staking and affiliate management capabilities.
39-47: Update toNewKeeperconstructor.The
NewKeeperconstructor has been updated to accept astakingKeeperas a parameter. This is necessary to initialize the newstakingKeeperfield in theKeeperstruct. The change is consistent with the addition of thestakingKeeperfield and is crucial for integrating staking functionalities.
359-363: Implementation ofUnsafeSetCachedStakedAmountmethod.This method allows for direct manipulation of the cached staked amount, which can be useful for administrative or testing purposes. However, as the name suggests, it is "unsafe" due to the lack of validation on the input data. It's important that this method is used cautiously and possibly restricted to non-production environments or secured through additional access controls.
| // GetTierForAffiliate returns the tier an affiliate address is qualified for. | ||
| // Assumes that the affiliate tiers are sorted by level in ascending order. | ||
| func (k Keeper) GetTierForAffiliate( | ||
| ctx sdk.Context, | ||
| affiliateAddr string, | ||
| ) ( | ||
| tierLevel uint32, | ||
| feeSharePpm uint32, | ||
| err error) { | ||
| affiliateTiers, err := k.GetAllAffiliateTiers(ctx) | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
| tiers := affiliateTiers.GetTiers() | ||
| numTiers := uint32(len(tiers)) | ||
| maxTierLevel := numTiers - 1 | ||
| currentTier := uint32(0) | ||
| referredVolume, err := k.GetReferredVolume(ctx, affiliateAddr) | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
|
|
||
| for index, tier := range tiers { | ||
| if referredVolume.Cmp(lib.BigU(tier.ReqReferredVolumeQuoteQuantums)) >= 0 { | ||
| // safe to do as tier cannot be negative | ||
| currentTier = uint32(index) | ||
| } | ||
| } | ||
|
|
||
| if currentTier == maxTierLevel { | ||
| return currentTier, tiers[currentTier].TakerFeeSharePpm, nil | ||
| } | ||
|
|
||
| numCoinsStaked := k.statsKeeper.GetStakedAmount(ctx, affiliateAddr) | ||
| for i := currentTier + 1; i < numTiers; i++ { | ||
| expMultiplier, _ := lib.BigPow10(lib.BaseDenomExponent) | ||
| reqStakedCoins := new(big.Int).Mul( | ||
| lib.BigU(tiers[i].ReqStakedWholeCoins), | ||
| expMultiplier, | ||
| ) | ||
| if numCoinsStaked.Cmp(reqStakedCoins) >= 0 { | ||
| currentTier = i | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| return currentTier, tiers[currentTier].TakerFeeSharePpm, nil | ||
| } |
There was a problem hiding this comment.
New method: GetTierForAffiliate.
This method determines the tier level for an affiliate based on referred volumes and staked amounts. It uses big.Int for handling large numbers and integrates with the statsKeeper to retrieve staked amounts. The method's logic is complex but well-implemented, using loops and conditions to determine the correct tier. However, there are comments suggesting optimizations and changes to the handling of big integers, which should be considered to improve performance and maintainability.
Consider the suggestions made in previous reviews regarding the use of optimized big integer operations and simplifying the logic to improve performance and readability.
- for i := currentTier + 1; i < numTiers; i++ {
- expMultiplier, _ := lib.BigPow10(lib.BaseDenomExponent)
- reqStakedCoins := new(big.Int).Mul(
- lib.BigU(tiers[i].ReqStakedWholeCoins),
- expMultiplier,
- )
- if numCoinsStaked.Cmp(reqStakedCoins) >= 0 {
- currentTier = i
- } else {
- break
- }
- }
+ // Optimized logic as suggested in previous reviewsCommittable suggestion was skipped due to low confidence.
| func TestGetReferredByEmptyAffiliate(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.AffiliatesKeeper | ||
|
|
||
| affiliate, exists := k.GetReferredBy(ctx, constants.AliceAccAddress.String()) | ||
| require.False(t, exists) | ||
| require.Equal(t, "", affiliate) | ||
| } |
There was a problem hiding this comment.
Add test for querying non-existent affiliate.
The function TestGetReferredByEmptyAffiliate correctly tests the scenario where a non-existent affiliate is queried. This is a good practice as it ensures the system behaves as expected in cases of missing data. However, consider adding a test case for when GetReferredBy is called with a referee who has no affiliate, as suggested in the previous comments. This will help ensure that all edge cases are covered and the system's robustness is verified.
Would you like me to help by adding this test case?
| func TestAddReferredVolume(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.AffiliatesKeeper | ||
|
|
||
| affiliate := "affiliate1" | ||
| initialVolume := big.NewInt(1000) | ||
| addedVolume := big.NewInt(500) | ||
|
|
||
| err := k.AddReferredVolume(ctx, affiliate, initialVolume) | ||
| require.NoError(t, err) | ||
|
|
||
| volume, err := k.GetReferredVolume(ctx, affiliate) | ||
| require.NoError(t, err) | ||
| require.Equal(t, initialVolume, volume) | ||
|
|
||
| err = k.AddReferredVolume(ctx, affiliate, addedVolume) | ||
| require.NoError(t, err) | ||
|
|
||
| updatedVolume, err := k.GetReferredVolume(ctx, affiliate) | ||
| require.NoError(t, err) | ||
| require.Equal(t, big.NewInt(1500), updatedVolume) | ||
| } |
There was a problem hiding this comment.
Verify correct handling of referred volumes.
The function TestAddReferredVolume effectively tests the addition of referred volumes and updates to the total volume. This is crucial for ensuring that the system accurately tracks and updates referred volumes as expected. Consider adding more granular error checks as suggested in the previous comments to enhance the test's ability to pinpoint issues.
| func TestGetReferredVolumeInvalidAffiliate(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.AffiliatesKeeper | ||
|
|
||
| affiliate := "malformed_address" | ||
| _, exists := k.GetReferredBy(ctx, affiliate) | ||
| require.False(t, exists) | ||
|
|
||
| affiliate = constants.AliceAccAddress.String() | ||
| _, exists = k.GetReferredBy(ctx, affiliate) | ||
| require.False(t, exists) | ||
| } |
There was a problem hiding this comment.
Enhance error handling in volume retrieval tests.
In TestGetReferredVolumeInvalidAffiliate, the test checks for the absence of a referred volume for invalid or non-existent affiliates. This is important for ensuring robust error handling. To improve this test, consider using more specific assertions to check the type or message of the expected error, as suggested in the previous comments. This will provide clearer feedback during test failures and help identify the exact nature of the errors encountered.
| // Test volume qualifies for tier 2 and stake qualifies for tier 3 | ||
| // should return tier 3 | ||
| func TestGetTierForAffiliate_VolumeAndStake(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.AffiliatesKeeper | ||
|
|
||
| affiliateTiers := types.DefaultAffiliateTiers | ||
| k.UpdateAffiliateTiers(ctx, affiliateTiers) | ||
| affiliate := constants.AliceAccAddress.String() | ||
| referee := constants.BobAccAddress.String() | ||
| stakingKeeper := tApp.App.StakingKeeper | ||
|
|
||
| err := stakingKeeper.SetDelegation(ctx, | ||
| stakingtypes.NewDelegation(constants.AliceAccAddress.String(), | ||
| constants.AliceValAddress.String(), math.LegacyNewDecFromBigInt( | ||
| new(big.Int).Mul( | ||
| big.NewInt(int64(types.DefaultAffiliateTiers.Tiers[0].ReqStakedWholeCoins)), | ||
| big.NewInt(1e18), | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| err = k.RegisterAffiliate(ctx, referee, affiliate) | ||
| require.NoError(t, err) | ||
|
|
||
| reqReferredVolume := big.NewInt(int64(affiliateTiers.Tiers[2].ReqReferredVolumeQuoteQuantums)) | ||
| err = k.AddReferredVolume(ctx, affiliate, reqReferredVolume) | ||
| require.NoError(t, err) | ||
|
|
||
| stakedAmount := new(big.Int).Mul( | ||
| big.NewInt(int64(affiliateTiers.Tiers[3].ReqStakedWholeCoins)), | ||
| big.NewInt(1e18), | ||
| ) | ||
| err = stakingKeeper.SetDelegation(ctx, | ||
| stakingtypes.NewDelegation(affiliate, | ||
| constants.AliceValAddress.String(), | ||
| math.LegacyNewDecFromBigInt(stakedAmount), | ||
| ), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| tierLevel, feeSharePpm, err := k.GetTierForAffiliate(ctx, affiliate) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, uint32(3), tierLevel) | ||
| require.Equal(t, affiliateTiers.Tiers[3].TakerFeeSharePpm, feeSharePpm) | ||
| } |
There was a problem hiding this comment.
Confirm tier determination logic with combined metrics.
TestGetTierForAffiliate_VolumeAndStake evaluates the tier determination logic based on both referred volume and staked amounts. This test is crucial for ensuring that the system accurately calculates the correct tier based on combined metrics. The implementation seems to follow the expected logic, but ensure that all possible combinations of volume and stake that could affect tier determination are tested. This will help guarantee that the tier calculation is robust and handles all scenarios appropriately.
| func TestGetStakedAmount(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| wholeCoinsToStake uint32 | ||
| }{ | ||
| { | ||
| name: "100 whole coins staked", | ||
| wholeCoinsToStake: 100, | ||
| }, | ||
| { | ||
| name: "100,000 whole coins staked", | ||
| wholeCoinsToStake: 100_000, | ||
| }, | ||
| { | ||
| name: "0 coins staked", | ||
| wholeCoinsToStake: 0, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| statsKeeper := tApp.App.StatsKeeper | ||
| stakingKeeper := tApp.App.StakingKeeper | ||
| expMultiplier, _ := lib.BigPow10(lib.BaseDenomExponent) | ||
| coinsToStakeQuantums := new(big.Int).Mul( | ||
| lib.BigU(tc.wholeCoinsToStake), | ||
| expMultiplier, | ||
| ) | ||
| delegation := stakingtypes.NewDelegation( | ||
| constants.AliceAccAddress.String(), constants.AliceValAddress.String(), | ||
| math.LegacyNewDecFromBigInt(coinsToStakeQuantums)) | ||
| err := stakingKeeper.SetDelegation(ctx, delegation) | ||
| require.NoError(t, err) | ||
|
|
||
| receivedCoins := statsKeeper.GetStakedAmount(ctx, constants.AliceAccAddress.String()) | ||
| require.Equal(t, coinsToStakeQuantums, receivedCoins) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetStakedAmount_Cache_Hit(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| statsKeeper := tApp.App.StatsKeeper | ||
| expMultiplier, _ := lib.BigPow10(lib.BaseDenomExponent) | ||
| coinsToStakeQuantums := new(big.Int).Mul( | ||
| lib.BigI(100), | ||
| expMultiplier, | ||
| ) | ||
| statsKeeper.UnsafeSetCachedStakedAmount(ctx, constants.AliceAccAddress.String(), &types.CachedStakeAmount{ | ||
| StakedAmount: dtypes.NewIntFromBigInt(coinsToStakeQuantums), | ||
| CachedAt: ctx.BlockTime().Unix(), | ||
| }) | ||
|
|
||
| receivedCoins := statsKeeper.GetStakedAmount(ctx, constants.AliceAccAddress.String()) | ||
| require.Equal(t, coinsToStakeQuantums, receivedCoins) | ||
| } | ||
|
|
||
| func TestGetStakedAmount_Cache_Miss(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| statsKeeper := tApp.App.StatsKeeper | ||
| stakingKeeper := tApp.App.StakingKeeper | ||
|
|
||
| expMultiplier, _ := lib.BigPow10(lib.BaseDenomExponent) | ||
| expiredWholeCoinsToStake := 100 | ||
| latestWholeCoinsToStake := 200 | ||
| expiredCoinsToStakeQuantums := new(big.Int).Mul( | ||
| lib.BigI(expiredWholeCoinsToStake), | ||
| expMultiplier, | ||
| ) | ||
| latestCoinsToStakeQuantums := new(big.Int).Mul( | ||
| lib.BigI(latestWholeCoinsToStake), | ||
| expMultiplier, | ||
| ) | ||
|
|
||
| // set expired delegation | ||
| statsKeeper.UnsafeSetCachedStakedAmount(ctx, constants.AliceAccAddress.String(), &types.CachedStakeAmount{ | ||
| StakedAmount: dtypes.NewIntFromBigInt(expiredCoinsToStakeQuantums), | ||
| CachedAt: ctx.BlockTime().Unix(), | ||
| }) | ||
|
|
||
| ctx = ctx.WithBlockTime(ctx.BlockTime().Add(time.Duration(epochstypes.StatsEpochDuration+1) * time.Second)) | ||
|
|
||
| delegation := stakingtypes.NewDelegation( | ||
| constants.AliceAccAddress.String(), constants.AliceValAddress.String(), | ||
| math.LegacyNewDecFromBigInt(latestCoinsToStakeQuantums)) | ||
| err := stakingKeeper.SetDelegation(ctx, delegation) | ||
| require.NoError(t, err) | ||
|
|
||
| receivedCoins := statsKeeper.GetStakedAmount(ctx, constants.AliceAccAddress.String()) | ||
| require.Equal(t, latestCoinsToStakeQuantums, receivedCoins) | ||
| } |
There was a problem hiding this comment.
Comprehensive Review of New Test Functions
The newly added test functions TestGetStakedAmount, TestGetStakedAmount_Cache_Hit, and TestGetStakedAmount_Cache_Miss are well-structured and address the functionality of the GetStakedAmount method under various conditions. Here are specific observations and suggestions for each test:
-
TestGetStakedAmount (Lines 299-339)
- The test cases cover a good range of scenarios, including edge cases like "0 coins staked".
- The use of
lib.BigPow10andlib.BigUfor handling large numbers is appropriate and ensures that the tests can handle different magnitudes of staking values. - Suggestion: Consider adding a test case for negative values to ensure that the method handles unexpected inputs gracefully.
-
TestGetStakedAmount_Cache_Hit (Lines 341-357)
- This test effectively checks the caching mechanism by pre-populating the cache and verifying that the cached value is returned.
- Suggestion: It would be beneficial to add assertions to check the cache's timestamp to ensure that the cache hit is within the expected time frame.
-
TestGetStakedAmount_Cache_Miss (Lines 359-393)
- The test setup for a cache miss by advancing the block time and updating the delegation is well implemented.
- Suggestion: Add more detailed comments explaining the steps involved in setting up the test conditions, especially for the cache expiration logic.
General Suggestions:
- Refactoring: Some common setup code could be refactored into helper functions to reduce redundancy and improve readability.
- Error Handling: Ensure that all potential error paths are tested, such as failures in setting delegations.
Overall, the tests are robust and align well with the functionality described in the PR. These tests will significantly aid in maintaining the reliability of the staking functionality.
| // GetStakedAmount returns the total staked amount for a delegator address. | ||
| // It maintains a cache to optimize performance. The function first checks | ||
| // if there's a cached value that hasn't expired. If found, it returns the | ||
| // cached amount. Otherwise, it calculates the staked amount by querying | ||
| // the staking keeper, caches the result, and returns the calculated amount | ||
| func (k Keeper) GetStakedAmount(ctx sdk.Context, | ||
| delegatorAddr string) *big.Int { | ||
| startTime := time.Now() | ||
| stakedAmount := big.NewInt(0) | ||
| store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.CachedStakeAmountKeyPrefix)) | ||
| bytes := store.Get([]byte(delegatorAddr)) | ||
|
|
||
| // return cached value if it's not expired | ||
| if bytes != nil { | ||
| var cachedStakedAmount types.CachedStakeAmount | ||
| k.cdc.MustUnmarshal(bytes, &cachedStakedAmount) | ||
| // sanity checks | ||
| if cachedStakedAmount.CachedAt < 0 { | ||
| panic("cachedStakedAmount.CachedAt is negative") | ||
| } | ||
| if ctx.BlockTime().Unix() < 0 { | ||
| panic("Invariant violation: ctx.BlockTime().Unix() is negative") | ||
| } | ||
| if cachedStakedAmount.CachedAt < 0 { | ||
| panic("Invariant violation: cachedStakedAmount.CachedAt is negative") | ||
| } | ||
| if cachedStakedAmount.CachedAt > ctx.BlockTime().Unix() { | ||
| panic("Invariant violation: cachedStakedAmount.CachedAt is greater than blocktime") | ||
| } | ||
| if ctx.BlockTime().Unix()-cachedStakedAmount.CachedAt <= types.StakedAmountCacheDurationSeconds { | ||
| stakedAmount.Set(cachedStakedAmount.StakedAmount.BigInt()) | ||
| metrics.IncrCounterWithLabels(metrics.StatsGetStakedAmountCacheHit, 1) | ||
| telemetry.MeasureSince( | ||
| startTime, | ||
| types.ModuleName, | ||
| metrics.StatsGetStakedAmountLatencyCacheHit, | ||
| metrics.Latency, | ||
| ) | ||
| return stakedAmount | ||
| } | ||
| } | ||
|
|
||
| metrics.IncrCounterWithLabels(metrics.StatsGetStakedAmountCacheMiss, 1) | ||
|
|
||
| // calculate staked amount | ||
| delegator, err := sdk.AccAddressFromBech32(delegatorAddr) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| // use math.MaxUint16 to get all delegations | ||
| delegations, err := k.stakingKeeper.GetDelegatorDelegations(ctx, delegator, math.MaxUint16) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| for _, delegation := range delegations { | ||
| stakedAmount.Add(stakedAmount, delegation.GetShares().RoundInt().BigInt()) | ||
| } | ||
|
|
||
| // update cache | ||
| cachedStakedAmount := types.CachedStakeAmount{ | ||
| StakedAmount: dtypes.NewIntFromBigInt(stakedAmount), | ||
| CachedAt: ctx.BlockTime().Unix(), | ||
| } | ||
| store.Set([]byte(delegatorAddr), k.cdc.MustMarshal(&cachedStakedAmount)) | ||
| telemetry.MeasureSince( | ||
| startTime, | ||
| types.ModuleName, | ||
| metrics.StatsGetStakedAmountLatencyCacheMiss, | ||
| metrics.Latency, | ||
| ) | ||
| return stakedAmount | ||
| } |
There was a problem hiding this comment.
Implementation of GetStakedAmount method.
This method efficiently handles the retrieval of staked amounts using a caching mechanism. The implementation includes several sanity checks to ensure the integrity of the cached data, such as checking for negative timestamps and ensuring the cache is not outdated. The use of telemetry and metrics to log cache hits and misses is a good practice, enhancing the observability of the method's performance.
However, there are redundant checks for cachedStakedAmount.CachedAt < 0 (lines 302 and 308). This should be cleaned up to avoid confusion and improve code readability.
Additionally, the panic in the case of errors (lines 332 and 337) could be replaced with more graceful error handling to prevent the application from crashing due to recoverable errors.
Consider removing redundant checks and improving error handling. Here's a suggested refactor for error handling:
- panic(err)
+ return nil, fmt.Errorf("failed to get delegator address: %w", err)And for the redundant checks:
- if cachedStakedAmount.CachedAt < 0 {
- panic("Invariant violation: cachedStakedAmount.CachedAt is negative")
- }Committable suggestion was skipped due to low confidence.
| DefaultAffiliateTiers = AffiliateTiers{ | ||
| Tiers: []AffiliateTiers_Tier{ | ||
| { | ||
| ReqReferredVolumeQuoteQuantums: 0, |
There was a problem hiding this comment.
Let's add human readable comment on the values (e.g. 1 million USDC, 10%) since these values are default parameters for the chain.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/x/affiliates/keeper/keeper.go (3 hunks)
- protocol/x/affiliates/keeper/keeper_test.go (2 hunks)
- protocol/x/affiliates/types/constants.go (1 hunks)
- protocol/x/stats/keeper/keeper_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/affiliates/keeper/keeper.go
- protocol/x/affiliates/keeper/keeper_test.go
- protocol/x/stats/keeper/keeper_test.go
Additional comments not posted (1)
protocol/x/affiliates/types/constants.go (1)
3-28: Well-documented constants for affiliate tiers.The
DefaultAffiliateTiersvariable is well-defined with clear, human-readable comments next to each value, enhancing the understandability of the code. The use of underscores in large numbers also improves readability. Great job addressing the previous review comments effectively.
Changelist
Adds keeper methods for x/affiliates and x/stats as described in doc
Test Plan
Added unit tests
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
New Features
CachedStakeAmountfor storing last calculated staked amounts, enhancing staking statistics.AffiliatesKeeper, including affiliate registration and referred volume tracking.Bug Fixes
Tests
AffiliatesKeeperandStatsKeeper, ensuring functionality correctness.GetStakedAmountmethod to validate staking operations.