[Tokenomics] Refactoring claim settlement to enable Token Logic Modules #708
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Olshansk
added
protocol
Olshansk
changed the title
WalkthroughRecent changes across various modules enhance clarity, maintainability, and functionality. Key updates include clearer output messages in the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (5)
x/tokenomics/keeper/msg_update_params_test.go (1)
95-95: Verify the handling of the additional return value.The additional return value from
TokenomicsKeeperWithActorAddrsis not consistently handled in the following instances:
x/tokenomics/keeper/query_params_test.go(lines 1, 8)x/tokenomics/keeper/msg_update_params_test.go(line 1)x/tokenomics/keeper/msg_server_test.go(line 1)x/tokenomics/module/genesis_test.go(line 1)testutil/keeper/tokenomics.go(line 1)Please ensure that the additional return value is correctly handled in these instances to avoid potential issues or inconsistencies in the test cases.
Analysis chain
Verify the handling of the additional return value.
Ensure that the additional return value from
TokenomicsKeeperWithActorAddrsis correctly handled and does not introduce errors or inconsistencies in the test case.
Verify the handling of the additional return value.
Ensure that the additional return value from
TokenomicsKeeperWithActorAddrsis correctly handled across all instances in the test files.
Verify the handling of the additional return value.
Ensure that the additional return value from
TokenomicsKeeperWithActorAddrsis correctly handled across all instances in the test files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of the additional return value from `TokenomicsKeeperWithActorAddrs`. # Test: Search for the function definition and usage. Expect: Correct handling of all return values. rg --type go -A 5 $'TokenomicsKeeperWithActorAddrs'Length of output: 4702
Script:
#!/bin/bash # Description: Verify the handling of the additional return value from `TokenomicsKeeperWithActorAddrs`. # List all instances of `TokenomicsKeeperWithActorAddrs` usage in test files and check if all return values are handled correctly. rg --type go -A 5 $'TokenomicsKeeperWithActorAddrs' | grep -E 'TokenomicsKeeperWithActorAddrs\(.*\)' | grep -v '_'Length of output: 193
Script:
#!/bin/bash # Description: Verify the handling of the additional return value from `TokenomicsKeeperWithActorAddrs`. # List all instances of `TokenomicsKeeperWithActorAddrs` usage in test files. rg --type go -A 5 $'TokenomicsKeeperWithActorAddrs'Length of output: 4702
x/tokenomics/module/module.go (1)
183-187: Issue Found:The
SupplierKeeperandServiceKeeperfields are defined but not utilized in the main implementation files. They are primarily used in test files, which indicates that their integration into the core logic might be incomplete.
- File:
x/tokenomics/module/module.go- Lines: 183-187
Please review the integration of
SupplierKeeperandServiceKeeperto ensure they are correctly utilized in the main implementation files.Analysis chain
LGTM! Verify the usage of new fields in the codebase.
The additions of
SupplierKeeperandServiceKeeperenhance the module's capabilities.Ensure that the new fields are correctly utilized in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the new fields `SupplierKeeper` and `ServiceKeeper`. # Test: Search for the new fields usage. Expect: Correct integration within the codebase. rg --type go 'SupplierKeeper|ServiceKeeper'Length of output: 6880
Script:
#!/bin/bash # Description: Verify the usage of `SupplierKeeper` and `ServiceKeeper` in function calls and method implementations. # Test: Search for the usage of `SupplierKeeper` in function calls and method implementations. rg --type go 'SupplierKeeper\.' -A 5 # Test: Search for the usage of `ServiceKeeper` in function calls and method implementations. rg --type go 'ServiceKeeper\.' -A 5Length of output: 1442
Script:
#!/bin/bash # Description: Verify the usage of `SupplierKeeper` and `ServiceKeeper` in main implementation files. # Test: Search for the usage of `SupplierKeeper` in main implementation files. rg --type go 'SupplierKeeper\.' -g '!*_test.go' -A 5 # Test: Search for the usage of `ServiceKeeper` in main implementation files. rg --type go 'ServiceKeeper\.' -g '!*_test.go' -A 5Length of output: 1474
config.yml (3)
98-98: Issue Found:inflation_rate_changeparameter is not utilized in the codebase.The
inflation_rate_changeparameter is defined inconfig.ymland mentioned indocs/static/openapi.yml, but there is no evidence of its usage in the functional parts of the codebase. This could indicate that the parameter is not integrated as intended.
config.yml:inflation_rate_change: "0.0"docs/static/openapi.yml: Multiple occurrencesPlease ensure that the
inflation_rate_changeparameter is correctly utilized within the codebase to reflect the intended minting strategy.Analysis chain
LGTM! Verify the correct usage of
inflation_rate_changeparameter.The addition of the
inflation_rate_changeparameter set to "0.0" is approved.Ensure the parameter is correctly utilized within the codebase.
Let's search for the usage of the
inflation_rate_changeparameter across the entire codebase to ensure it's correctly integrated and utilized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `inflation_rate_change` parameter. # Test: Search for the parameter usage. Expect: Correct integration within the codebase. rg --type yaml 'inflation_rate_change'Length of output: 383
Script:
#!/bin/bash # Description: Verify the correct usage of `inflation_rate_change` parameter across the codebase. # Test: Search for the parameter usage in the entire codebase. Expect: Correct integration within the codebase. rg 'inflation_rate_change'Length of output: 371
100-100: Theinflation_minparameter is defined but not utilized within the codebase.
config.yml: The parameterinflation_minis set to "0.0".- No other non-documentation files reference
inflation_min.Please ensure that the parameter is integrated and utilized where necessary.
Analysis chain
LGTM! Verify the correct usage of
inflation_minparameter.The addition of the
inflation_minparameter set to "0.0" is approved.Ensure the parameter is correctly utilized within the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `inflation_min` parameter. # Test: Search for the parameter usage. Expect: Correct integration within the codebase. rg --type yaml 'inflation_min'Length of output: 327
Script:
#!/bin/bash # Description: Verify the correct usage of `inflation_min` parameter in non-documentation files. # Test: Search for the parameter usage in non-documentation files. Expect: Correct integration within the codebase. rg 'inflation_min' --glob '!docs/**' --glob '!*.md'Length of output: 91
99-99: Issue Found:inflation_maxparameter is not used in the implementationThe
inflation_maxparameter is defined inconfig.ymland documented indocs/static/openapi.yml, but it is not used in the implementation files. This could indicate a missing integration or an oversight.
config.yml:inflation_max: "0.0"docs/static/openapi.yml: Multiple mentions ofinflation_maxPlease review and ensure that the
inflation_maxparameter is correctly utilized within the codebase.Analysis chain
LGTM! Verify the correct usage of
inflation_maxparameter.The addition of the
inflation_maxparameter set to "0.0" is approved.Ensure the parameter is correctly utilized within the codebase.
Let's gather more information about the usage of
inflation_maxindocs/static/openapi.ymlto ensure it is correctly integrated.
To ensure the
inflation_maxparameter is correctly integrated within the implementation, we need to search for its usage in the codebase.
Let's correct the file type specification and re-run the search for the usage of
inflation_maxin the implementation files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `inflation_max` parameter. # Test: Search for the parameter usage. Expect: Correct integration within the codebase. rg --type yaml 'inflation_max'Length of output: 327
Script:
#!/bin/bash # Extract and review the context around `inflation_max` in `docs/static/openapi.yml`. rg --type yaml -A 5 -B 5 'inflation_max' docs/static/openapi.ymlLength of output: 2038
Script:
#!/bin/bash # Search for the usage of `inflation_max` in the implementation files. rg 'inflation_max' --type not yamlLength of output: 62
Script:
#!/bin/bash # Search for the usage of `inflation_max` in the implementation files, excluding YAML files. rg 'inflation_max' --glob '!*.yaml'Length of output: 332
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
| @@ -177,22 +171,18 @@ genesis: | |||
| supplierList: | |||
| - address: pokt19a3t4yunp0dlpfjrp7qwnzwlrzd5fzs2gjaaaj | |||
| services: | |||
| - endpoints: | |||
| - service: | |||
| id: anvil | |||
There was a problem hiding this comment.
Only referencing the Service.Id makes sense and also raises the question about embedding full entities into others (the whole Service is embedded into the Supplier in this case) .
It pushes devs to use the embedded data instead of querying the source of truth.
You already solved one of these which was about the service's ComputeUnitsPerRelay where the dev used the value embedded in the Application.Services.ComputeUnitsPerRelay instead of fetching the Service which is the source of truth.
We should (at some point) only embed ids and enforce fetching the rest of the data from the SOT.
There was a problem hiding this comment.
Updated some TODOs. PTAL
This also made me realize that onchain data structures are like micro-services.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
proto/poktroll/shared/service.proto (1)
33-33: Consider removing redundantTODO_MAINNETcomments.The comments suggesting avoiding embedding the full
Servicemessage are redundant since the changes have already been made.- // TODO_MAINNET: Avoid embedding the full Service because we just need the ID.Also applies to: 43-43
e2e/tests/init_test.go (1)
704-704: Improve the error message ingetServiceComputeUnitsPerRelay.The error message should be more specific to help with debugging.
- require.NoError(s, err, "error getting shared module params") + require.NoError(s, err, "error getting service details for service ID %s", serviceId)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
api/poktroll/shared/service.pulsar.go (1)
2926-2926: Potential Issue: FullServiceStruct May Be Embedded inSupplierServiceConfigThe
SupplierServiceConfigstruct appears to have a methodGetServicethat accesses theServicestruct, suggesting that the fullServicestruct is embedded or accessed. This could contradict the design intention mentioned in the comment to avoid embedding the fullServicestruct. Consider reviewing the codebase for dependencies on theServicestruct withinSupplierServiceConfig.
- File:
api/poktroll/shared/service.pulsar.go- Method:
SupplierServiceConfig.GetService()Analysis chain
LGTM! Verify the impact of not embedding the full
Servicestruct.The comment to avoid embedding the full
Servicestruct is consistent with the design decision inApplicationServiceConfig. Ensure that this change does not affect existing functionality where the fullServicemight have been used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the full `Service` struct is used in `SupplierServiceConfig` related code. # Test: Search for `SupplierServiceConfig` usage. Expect: No dependencies on full `Service` struct. rg --type go 'SupplierServiceConfig'Length of output: 26006
This was a collaboration between @RawthiL and @Olshansk to go through the notebooks and understand how the repo can be simplified into an initial version that can go to production. See pokt-network/poktroll#708 for reference. --------- Co-authored-by: Ramiro Rodriguez Colmeiro <[email protected]>
…es (#708) _NOTE: No actual changes in minting/burning amounts were created_ - Rename `SettleSessionAccounting` to `ProcessTokenLogicModules - Modularize the `TokenLogicModules` so we can start adding the different modules - Introduced a global mint + distribution function (set to 0 right now) as a foundation for how we can distribute minted rewards across different actors - The core logic is in `x/tokenomics/keeper/token_logic_modules.go` Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Redouane Lakrache <[email protected]> Co-authored-by: Dima K. <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
NOTE: No actual changes in minting/burning amounts were created
SettleSessionAccountingto `ProcessTokenLogicModulesTokenLogicModulesso we can start adding the different modulesrewards across different actors
x/tokenomics/keeper/token_logic_modules.goIssue
Type of change
Select one or more:
Testing
make docusaurus_start; only needed if you make doc changesmake go_develop_and_testmake test_e2edevnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
ServiceKeeperin the Tokenomics module for better service management.Refactor
SupplierKeeperandServiceKeeperinterfaces to expand functionality.Tests