[TODO] refactor: query expiring claims w/ index #671
Conversation
bryanchriswhite
added
on-chain
WalkthroughThe changes integrate a session keeper into the tokenomics module, refactor window size calculations in shared sessions, and improve error handling and efficiency in settling pending claims. This involves modifying several structs and functions to include and utilize the new session keeper dependency, impacting how claims and proofs are managed. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
bryanchriswhite
added
push-image
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 671) |
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
x/tokenomics/module/module.go (1)
185-185: Add a comment for the new SessionKeeper field.To maintain consistency and readability, add a comment explaining the purpose of the new
SessionKeeperfield in theModuleInputsstruct.+ // SessionKeeper manages session-related functionalities. SessionKeeper types.SessionKeeper
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- testutil/integration/app.go (1 hunks)
- testutil/keeper/tokenomics.go (3 hunks)
- x/shared/session.go (3 hunks)
- x/shared/session_test.go (1 hunks)
- x/tokenomics/keeper/keeper.go (4 hunks)
- x/tokenomics/keeper/settle_pending_claims.go (3 hunks)
- x/tokenomics/module/module.go (2 hunks)
- x/tokenomics/types/expected_keepers.go (4 hunks)
Additional comments not posted (14)
x/tokenomics/keeper/keeper.go (2)
31-33: LGTM! Ensure correct usage ofsessionKeeperandsharedQuerier.The additions of
sessionKeeperandsharedQuerierfields in theKeeperstruct are approved.However, ensure that all usages of these fields are correct and consistent throughout the codebase.
47-54: LGTM! Verify the initialization ofsharedQuerier.The initialization of
sharedQuerierin theNewKeeperfunction is approved.However, ensure that the initialization of
sharedQuerierusingprooftypes.NewSharedKeeperQueryClientis correct and consistent throughout the codebase.Verification successful
The initialization of
sharedQuerierusingprooftypes.NewSharedKeeperQueryClientappears consistently across multiple files. However, to ensure correctness, let's verify the context and parameters in the identified files.
Verified: Initialization of
sharedQuerierThe initialization of
sharedQuerierusingprooftypes.NewSharedKeeperQueryClientis consistent and correct across the codebase. Each instance passes the appropriate parameters (sharedKeeperandsessionKeeper).
x/tokenomics/keeper/keeper.gox/proof/keeper/msg_server_submit_proof_test.gox/proof/keeper/keeper.gotestutil/integration/app.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct initialization of `sharedQuerier` using `prooftypes.NewSharedKeeperQueryClient`. # Test: Search for the initialization of `sharedQuerier`. Expect: Only valid initializations. rg --type go --word-regexp 'NewSharedKeeperQueryClient'Length of output: 958
Script:
#!/bin/bash # Display context around each usage of NewSharedKeeperQueryClient to verify correctness. rg --type go --word-regexp 'NewSharedKeeperQueryClient' -A 5 -B 5Length of output: 6114
x/tokenomics/types/expected_keepers.go (2)
43-44: LGTM! Ensure correct implementation ofAllClaimsmethod.The addition of the
AllClaimsmethod to theProofKeeperinterface is approved.However, ensure that the
AllClaimsmethod is correctly implemented and used throughout the codebase.Verification successful
The
AllClaimsmethod is implemented and used correctly in the codebase.The method is defined in the
Keeperand used consistently across various modules and tests. Here are the key points:
Definition:
x/proof/keeper/query_claim.go:func (k Keeper) AllClaims(ctx context.Context, req *types.QueryAllClaimsRequest) (*types.QueryAllClaimsResponse, error)Usage:
x/tokenomics/keeper/settle_pending_claims.gox/proof/types/query.pb.gw.gox/proof/types/query.pb.gox/proof/module/query_claim.go- Various test files
The implementation and usage of the
AllClaimsmethod appear consistent and correct across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct implementation and usage of `AllClaims` method. # Test: Search for the implementation of `AllClaims` method. Expect: Only valid implementations. ast-grep --lang go --pattern $'func ($_).AllClaims($_, $_)' # Test: Search for the usage of `AllClaims` method. Expect: Only valid usages. rg --type go --word-regexp 'AllClaims'Length of output: 4454
60-64: LGTM! Ensure correct implementation ofSessionKeepermethods.The addition of the
SessionKeeperinterface with methodsGetSession,GetBlockHash, andStoreBlockHashis approved.However, ensure that these methods are correctly implemented and used throughout the codebase.
x/shared/session_test.go (2)
223-224: LGTM! Ensure correct usage ofGetProofWindowCloseOffsetBlocksmethod.The replacement of
GetProofWindowSizeBlockswithGetProofWindowCloseOffsetBlocksmethod fromsharedParamsis approved.However, ensure that the
GetProofWindowCloseOffsetBlocksmethod is correctly used throughout the codebase.Verification successful
Verified: Correct usage of
GetProofWindowCloseOffsetBlocksmethod.The method
GetProofWindowCloseOffsetBlocksis used correctly throughout the codebase. Each instance appropriately retrieves and uses theProofWindowCloseOffsetBlocksvalue.
x/shared/types/params.pb.go: Method implementation.x/shared/session_test.go: Test assertions.x/shared/session.go: Calculation of proof window close height and distribution window size.x/tokenomics/keeper/settle_pending_claims.go: Calculation involving block heights.api/poktroll/shared/params.pulsar.go: Method implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of `GetProofWindowCloseOffsetBlocks` method. # Test: Search for the usage of `GetProofWindowCloseOffsetBlocks` method. Expect: Only valid usages. rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks'Length of output: 646
Script:
#!/bin/bash # Extracting context around usages of GetProofWindowCloseOffsetBlocks # Extract context in x/shared/types/params.pb.go echo "Context in x/shared/types/params.pb.go" rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/types/params.pb.go # Extract context in x/shared/session_test.go echo "Context in x/shared/session_test.go" rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session_test.go # Extract context in x/shared/session.go echo "Context in x/shared/session.go" rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session.go # Extract context in x/tokenomics/keeper/settle_pending_claims.go echo "Context in x/tokenomics/keeper/settle_pending_claims.go" rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/tokenomics/keeper/settle_pending_claims.go # Extract context in api/poktroll/shared/params.pulsar.go echo "Context in api/poktroll/shared/params.pulsar.go" rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 api/poktroll/shared/params.pulsar.goLength of output: 3255
220-221: LGTM! Ensure correct usage ofGetClaimWindowCloseOffsetBlocksmethod.The replacement of
GetClaimWindowSizeBlockswithGetClaimWindowCloseOffsetBlocksmethod fromsharedParamsis approved.However, ensure that the
GetClaimWindowCloseOffsetBlocksmethod is correctly used throughout the codebase.Verification successful
Verify correct usage of
GetClaimWindowCloseOffsetBlocksmethod.The
GetClaimWindowCloseOffsetBlocksmethod is used in the following files:
x/shared/session_test.gox/shared/types/params.pb.gox/shared/session.gox/tokenomics/keeper/settle_pending_claims.goapi/poktroll/shared/params.pulsar.goWe need to ensure that this method is used correctly in each of these files.
Verified: Correct usage of
GetClaimWindowCloseOffsetBlocksmethodThe method
GetClaimWindowCloseOffsetBlocksis correctly used throughout the codebase in the following files:
x/shared/session_test.gox/shared/types/params.pb.gox/shared/session.gox/tokenomics/keeper/settle_pending_claims.goapi/poktroll/shared/params.pulsar.goEach instance adheres to the intended functionality of the method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of `GetClaimWindowCloseOffsetBlocks` method. # Test: Search for the usage of `GetClaimWindowCloseOffsetBlocks` method. Expect: Only valid usages. rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks'Length of output: 677
Script:
#!/bin/bash # Extract surrounding lines for context around the usage of GetClaimWindowCloseOffsetBlocks method. # Get 10 lines of context before and after the usage of the method for better understanding. rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 10 -B 10Length of output: 8220
x/shared/session.go (2)
142-144: LGTM! Ensure correct usage ofGetProofWindowCloseOffsetBlocksmethod.The replacement of
GetProofWindowSizeBlockswithGetProofWindowCloseOffsetBlocksmethod fromsharedParamsinGetEarliestSupplierProofCommitHeightis approved.However, ensure that the
GetProofWindowCloseOffsetBlocksmethod is correctly used throughout the codebase.Verification successful
The usage of
GetProofWindowCloseOffsetBlocksis correct throughout the codebase.The method is used appropriately in the following files:
x/shared/session_test.gox/tokenomics/keeper/settle_pending_claims.gox/shared/session.goNo issues found with the current implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of `GetProofWindowCloseOffsetBlocks` method. # Test: Search for the usage of `GetProofWindowCloseOffsetBlocks` method. Expect: Only valid usages. rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks'Length of output: 646
Script:
#!/bin/bash # Description: Gather detailed context around each usage of `GetProofWindowCloseOffsetBlocks` # Get context around the usage in session_test.go rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session_test.go # Get context around the usage in settle_pending_claims.go rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/tokenomics/keeper/settle_pending_claims.go # Get context around the usage in session.go rg --type go --word-regexp 'GetProofWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session.goLength of output: 2213
121-123: LGTM! Ensure correct usage ofGetClaimWindowCloseOffsetBlocksmethod.The replacement of
GetClaimWindowSizeBlockswithGetClaimWindowCloseOffsetBlocksmethod fromsharedParamsinGetEarliestSupplierClaimCommitHeightis approved.However, ensure that the
GetClaimWindowCloseOffsetBlocksmethod is correctly used throughout the codebase.Verification successful
Verified: Correct usage of
GetClaimWindowCloseOffsetBlocksmethod.The
GetClaimWindowCloseOffsetBlocksmethod is correctly utilized across the codebase, ensuring consistency and correctness in fetching theClaimWindowCloseOffsetBlocksparameter.
x/tokenomics/keeper/settle_pending_claims.gox/shared/session_test.gox/shared/types/params.pb.gox/shared/session.goapi/poktroll/shared/params.pulsar.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of `GetClaimWindowCloseOffsetBlocks` method. # Test: Search for the usage of `GetClaimWindowCloseOffsetBlocks` method. Expect: Only valid usages. rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks'Length of output: 677
Script:
#!/bin/bash # Description: Verify the context of `GetClaimWindowCloseOffsetBlocks` method usage. # Display context around the usage in `x/tokenomics/keeper/settle_pending_claims.go` rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 5 -B 5 x/tokenomics/keeper/settle_pending_claims.go # Display context around the usage in `x/shared/session_test.go` rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session_test.go # Display context around the usage in `x/shared/types/params.pb.go` rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/types/params.pb.go # Display context around the usage in `x/shared/session.go` rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 5 -B 5 x/shared/session.go # Display context around the usage in `api/poktroll/shared/params.pulsar.go` rg --type go --word-regexp 'GetClaimWindowCloseOffsetBlocks' -A 5 -B 5 api/poktroll/shared/params.pulsar.goLength of output: 2988
x/tokenomics/module/module.go (1)
211-211: LGTM! The new dependency injection is correctly implemented.The
ProvideModulefunction correctly initializes thesessionKeeperas a new dependency.x/tokenomics/keeper/settle_pending_claims.go (1)
164-206: Ensure proper pagination handling forgetExpiringClaims.The pagination logic seems correct, but ensure that the
Offsetis properly managed to avoid infinite loops or missed claims.testutil/keeper/tokenomics.go (3)
158-160: LGTM! The session keeper mock is correctly added.The mock for
sessionKeeperis correctly added.
171-171: LGTM! The session keeper is correctly integrated.The
sessionKeeperis correctly integrated into theTokenomicsKeeperinitialization.
338-338: LGTM! The session keeper is correctly initialized.The
sessionKeeperis correctly initialized in theNewTokenomicsModuleKeepersfunction.testutil/integration/app.go (1)
396-396: Ensure the new session keeper is correctly passed and used.The
sessionKeeperparameter is added but ensure it's correctly passed and used.Verification successful
Ensure the new session keeper is correctly passed and used.
The
sessionKeeperparameter is added and correctly used within thetestutil/integration/app.gofile. ThesessionKeeperis instantiated and used as expected in various parts of the file.
sessionKeeperis instantiated and used in the following locations:
sessionkeeper.NewKeepersessiontypes.RegisterMsgServersessiontypes.RegisterQueryServersessionKeeper.SetParamsprooftypes.NewSharedKeeperQueryClientScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `sessionKeeper` parameter. # Test: Search for the function usage. Expect: Proper passing and usage of `sessionKeeper`. rg --type go -A 5 'NewCompleteIntegrationApp'Length of output: 2159
Script:
#!/bin/bash # Description: Verify the usage of the `sessionKeeper` parameter within the `NewCompleteIntegrationApp` function. # Test: Search for the `sessionKeeper` variable usage within the `NewCompleteIntegrationApp` function. rg --type go -A 30 'func NewCompleteIntegrationApp' testutil/integration/app.go | rg 'sessionKeeper'Length of output: 100
Script:
#!/bin/bash # Description: Verify the usage of the `sessionKeeper` parameter within the `NewCompleteIntegrationApp` function and the entire file. # Test: Search for the complete definition of the `NewCompleteIntegrationApp` function. rg --type go -A 100 'func NewCompleteIntegrationApp' testutil/integration/app.go # Test: Search for any usage of the `sessionKeeper` within the `testutil/integration/app.go` file. rg --type go 'sessionKeeper' testutil/integration/app.goLength of output: 3648
| expiringClaims, err := k.getExpiringClaims(ctx) | ||
| if err != nil { | ||
| return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err | ||
| } |
There was a problem hiding this comment.
Ensure relaysPerServiceMap and computeUnitsPerServiceMap are initialized before use.
The variables relaysPerServiceMap and computeUnitsPerServiceMap should be initialized before being used in the return statement to avoid potential nil map issues.
expiringClaims, err := k.getExpiringClaims(ctx)
if err != nil {
+ relaysPerServiceMap = make(map[string]uint64)
+ computeUnitsPerServiceMap = make(map[string]uint64)
return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expiringClaims, err := k.getExpiringClaims(ctx) | |
| if err != nil { | |
| return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err | |
| } | |
| expiringClaims, err := k.getExpiringClaims(ctx) | |
| if err != nil { | |
| relaysPerServiceMap = make(map[string]uint64) | |
| computeUnitsPerServiceMap = make(map[string]uint64) | |
| return 0, 0, relaysPerServiceMap, computeUnitsPerServiceMap, err | |
| } |
| Offset: uint64(len(expiringClaims)), | ||
| }, | ||
| Filter: &prooftypes.QueryAllClaimsRequest_SessionEndHeight{ | ||
| SessionEndHeight: uint64(previousSessionEndHeight), |
There was a problem hiding this comment.
Wouldn't it be easier to just use SessionNumber for filtering?
There was a problem hiding this comment.
I'm not sure but at the moment we don't have any such index; we have:
oneof filter {
string supplier_address= 2;
string session_id = 3;
uint64 session_end_height = 4;
}Even if we had a session_number index, we would still have to do something more sophistocated than simply currentHeight / num_blocks_per_session. Do you think it's worth adding a TODO_CONSIDERATION or something?
There was a problem hiding this comment.
Maybe.
My concern is about the potential case where the session we want to settle is n-2 or older (relative to the current session).
If claim and proof window offsets do not allow that, then we should be fine, Otherwise we should consider other means to retrieve the session we want to settle.
There was a problem hiding this comment.
This method will be called for each block, progressively "draining" all expiring claims as the current height climbs. Since this is part of consensus, all sessions considered here MUST be from the same, most recently expiring session number.
…ation-use-index * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/tokenomics/keeper/settle_pending_claims.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_pending_claims.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testutil/keeper/tokenomics.go (3 hunks)
- x/tokenomics/keeper/settle_pending_claims.go (3 hunks)
- x/tokenomics/types/expected_keepers.go (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- testutil/keeper/tokenomics.go
- x/tokenomics/keeper/settle_pending_claims.go
- x/tokenomics/types/expected_keepers.go
…ation-overserviced * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
…-root * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
Summary
sessionEndHeightindex of claims to optimize the querying of expiring claims.GetEarliestSupplierClaimCommitHeight()andGetEarliestSupplierProofCommitHeight()by eliminating incorrect and redundant functionsGetClaimWindowSizeBlocks()andGetProofWindowSizeBlocks()Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
SessionKeeperfor improved session handling within the tokenomics module.SettlePendingClaimsfunction to handle errors, include shared parameters, and use pagination for claim retrieval.Refactor
SessionKeeperin initialization functions.Bug Fixes