[Supplier] Implement supplier revenue share #729
Conversation
… refactor/non-custodial-staking-tests
… refactor/non-custodial-staking-tests
… refactor/non-custodial-staking-tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (6)
x/supplier/types/message_unstake_supplier.go (1)
17-19: Clarify error message inValidateBasic.The error message "invalid address address" is redundant. Consider simplifying it to "invalid signer address" for clarity.
return ErrSupplierInvalidAddress.Wrapf("invalid signer address (%s)", err)x/supplier/module/tx_unstake_supplier.go (2)
29-33: Clarify the comment regarding the address.The comment "address is the must be the owner or operator address of the supplier" is unclear. Consider rephrasing it to improve readability.
// The address must be the owner or operator address of the supplier.
15-21: Documentation and help messages need updating for<operator_address>.The
<operator_address>argument is present in thetx_unstake_supplier.gofile, but there are no corresponding updates in markdown documentation files. Ensure all related documentation and help messages are updated to include<operator_address>for consistency.
- Update markdown documentation files to reflect the changes in command usage.
Analysis chain
Ensure consistency in command usage documentation.
The command usage and description have been updated to include
<operator_address>. Ensure that all related documentation and help messages reflect this change for consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all documentation and help messages are updated to include `<operator_address>`. # Test: Search for the command usage in documentation files. Expect: Consistent usage of `<operator_address>`. rg --type markdown --type go '<operator_address>'Length of output: 366
x/supplier/keeper/unbond_suppliers.go (1)
42-47: Improve error handling for owner address parsing.The error handling for parsing the owner address is appropriate, but consider adding more context to the error message to aid in debugging. For example, include the supplier ID or name if available.
logger.Error(fmt.Sprintf("could not parse the owner address %s for supplier %s", supplier.OwnerAddress, supplier.ID))x/supplier/types/message_stake_supplier.go (1)
31-34: Validate owner address with clear error messages.The validation for the owner address is correctly implemented, but consider logging the error with more context, such as which supplier's address failed validation.
logger.Error(fmt.Sprintf("invalid owner address %s for supplier %s; (%v)", msg.OwnerAddress, msg.Address, err))load-testing/tests/relays_stress_helpers_test.go (1)
788-789: Potential issue with repeated use ofsupplier.address.The function
NewMsgStakeSupplierexpects three distinct address parameters:signerAddress,ownerAddress, andsupplierAddress. The repeated use ofsupplier.addressin the test file might be incorrect and should be reviewed to ensure it aligns with the intended logic.
- File:
load-testing/tests/relays_stress_helpers_test.go- Lines: 788-789
Analysis chain
Verify repeated use of
supplier.address.The
NewMsgStakeSupplierfunction now receivessupplier.addressmultiple times. Ensure this aligns with the updated function signature and intended logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the function signature of `NewMsgStakeSupplier` to ensure the repeated use of `supplier.address` is intentional. # Test: Search for the `NewMsgStakeSupplier` function definition. Expect: The function signature should match the usage in the code. ast-grep --lang go --pattern 'func NewMsgStakeSupplier($_, $_, $_, $_, $_) $_'Length of output: 1129
red-0ne
changed the base branch from
main
to
refactor/non-custodial-staking-tests
| @@ -43,6 +45,8 @@ poktrolld tx supplier stake-supplier \ | |||
|
|
|||
| ## Configuration | |||
|
|
|||
| <!-- TODO(#716): `owner_address` will be defined once non-custodial is merged --> | |||
There was a problem hiding this comment.
Added rev share specific usage of the owner_address, other content will be available once #720 is merged.
red-0ne
changed the base branch from
refactor/non-custodial-staking-tests
to
main
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
proto/poktroll/shared/service.proto (1)
46-46: Consider renaming the structure toServiceRevenueShare.The existing comment from a previous review suggests renaming the structure to
ServiceRevenueSharefor clarity and consistency. This change aligns with the naming conventions used throughout the file.- repeated ServiceRevenueShare rev_share = 3; + repeated ServiceRevenueShare rev_share = 3; // Consider renaming to `ServiceRevenueShare`x/shared/helpers/service_configs.go (2)
124-128: Clarify the error message for invalid rev share percentage.The error message for invalid revenue share percentage could be more specific about the valid range.
- "invalid rev share value %v; must be between 0 and 100", + "invalid rev share value %v; must be greater than 0 and less than or equal to 100",
134-139: Clarify the error message for invalid rev share percentage sum.The error message for the sum of revenue share percentages could be more informative by specifying the expected total.
- "invalid rev share percentage sum %v; must be equal to %v", + "invalid total rev share percentage sum %v; expected total is %v",
x/shared/helpers/service_configs.go
Outdated
| func ValidateServiceRevShare(revShareList []*sharedtypes.ServiceRevenueShare) error { | ||
| revSharePercentageSum := float32(0) | ||
|
|
||
| if len(revShareList) == 0 { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("no rev share configurations") | ||
| } | ||
|
|
||
| for _, revShare := range revShareList { | ||
| if revShare == nil { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share cannot be nil") | ||
| } | ||
|
|
||
| if len(revShare.Address) == 0 { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share address cannot be empty") | ||
| } | ||
|
|
||
| // Validate the revshare address | ||
| if revShare.Address == "" { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf("rev share address cannot be empty: %v", revShare) | ||
| } | ||
|
|
||
| if _, err := sdk.AccAddressFromBech32(revShare.Address); err != nil { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf("invalid rev share address %s; (%v)", revShare.Address, err) | ||
| } | ||
|
|
||
| if revShare.RevSharePercentage <= 0 || revShare.RevSharePercentage > 100 { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | ||
| "invalid rev share value %v; must be between 0 and 100", | ||
| revShare.RevSharePercentage, | ||
| ) | ||
| } | ||
|
|
||
| revSharePercentageSum += revShare.RevSharePercentage | ||
| } | ||
|
|
||
| if revSharePercentageSum != requiredRevSharePercentageSum { | ||
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | ||
| "invalid rev share percentage sum %v; must be equal to %v", | ||
| revSharePercentageSum, | ||
| requiredRevSharePercentageSum, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove redundant address check in ValidateServiceRevShare.
The check for revShare.Address == "" is redundant since len(revShare.Address) == 0 covers the same condition.
- if revShare.Address == "" {
- return sharedtypes.ErrSharedInvalidRevShare.Wrapf("rev share address cannot be empty: %v", revShare)
- }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.
| func ValidateServiceRevShare(revShareList []*sharedtypes.ServiceRevenueShare) error { | |
| revSharePercentageSum := float32(0) | |
| if len(revShareList) == 0 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("no rev share configurations") | |
| } | |
| for _, revShare := range revShareList { | |
| if revShare == nil { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share cannot be nil") | |
| } | |
| if len(revShare.Address) == 0 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share address cannot be empty") | |
| } | |
| // Validate the revshare address | |
| if revShare.Address == "" { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf("rev share address cannot be empty: %v", revShare) | |
| } | |
| if _, err := sdk.AccAddressFromBech32(revShare.Address); err != nil { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf("invalid rev share address %s; (%v)", revShare.Address, err) | |
| } | |
| if revShare.RevSharePercentage <= 0 || revShare.RevSharePercentage > 100 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | |
| "invalid rev share value %v; must be between 0 and 100", | |
| revShare.RevSharePercentage, | |
| ) | |
| } | |
| revSharePercentageSum += revShare.RevSharePercentage | |
| } | |
| if revSharePercentageSum != requiredRevSharePercentageSum { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | |
| "invalid rev share percentage sum %v; must be equal to %v", | |
| revSharePercentageSum, | |
| requiredRevSharePercentageSum, | |
| ) | |
| } | |
| func ValidateServiceRevShare(revShareList []*sharedtypes.ServiceRevenueShare) error { | |
| revSharePercentageSum := float32(0) | |
| if len(revShareList) == 0 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("no rev share configurations") | |
| } | |
| for _, revShare := range revShareList { | |
| if revShare == nil { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share cannot be nil") | |
| } | |
| if len(revShare.Address) == 0 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrap("rev share address cannot be empty") | |
| } | |
| if _, err := sdk.AccAddressFromBech32(revShare.Address); err != nil { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf("invalid rev share address %s; (%v)", revShare.Address, err) | |
| } | |
| if revShare.RevSharePercentage <= 0 || revShare.RevSharePercentage > 100 { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | |
| "invalid rev share value %v; must be between 0 and 100", | |
| revShare.RevSharePercentage, | |
| ) | |
| } | |
| revSharePercentageSum += revShare.RevSharePercentage | |
| } | |
| if revSharePercentageSum != requiredRevSharePercentageSum { | |
| return sharedtypes.ErrSharedInvalidRevShare.Wrapf( | |
| "invalid rev share percentage sum %v; must be equal to %v", | |
| revSharePercentageSum, | |
| requiredRevSharePercentageSum, | |
| ) | |
| } |
|
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 729) |
github-actions
bot
added
devnet
push-image
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
docusaurus/docs/operate/configs/supplier_staking_config.md (2)
92-92: Fix punctuation for consistency.Ensure consistent punctuation usage in section headers for clarity.
- _`Optional`_, _`Non-empty`_ + _`Optional`_, _`Non-empty`_.Tools
LanguageTool
[uncategorized] ~92-~92: Loose punctuation mark.
Context: ...default_rev_share_percent_Optional_, _Non-empty`_ ```yaml default_rev_shar...(UNLIKELY_OPENING_PUNCTUATION)
226-227: Add missing comma for clarity.Consider adding a comma for clarity in the list of conditions.
- The revenue share values MUST be strictly positive decimals with a maximum value of 100 and a total sum of 100 across all the `shareholder_address`es. + The revenue share values MUST be strictly positive decimals, with a maximum value of 100, and a total sum of 100 across all the `shareholder_address`es.Tools
LanguageTool
[uncategorized] ~226-~226: Possible missing comma found.
Context: ... share values MUST be strictly positive decimals with a maximum value of 100 and a total...(AI_HYDRA_LEO_MISSING_COMMA)
| if err := ValidateServiceRevShare(serviceConfig.RevShare); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Improve error message clarity in ValidateSupplierServiceConfigs.
Consider providing more context in the error message returned by ValidateServiceRevShare. This can help in debugging issues related to specific service configurations.
- return err
+ return fmt.Errorf("invalid revenue share configuration for service %v: %w", serviceConfig.Service, 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.
| if err := ValidateServiceRevShare(serviceConfig.RevShare); err != nil { | |
| return err | |
| } | |
| if err := ValidateServiceRevShare(serviceConfig.RevShare); err != nil { | |
| return fmt.Errorf("invalid revenue share configuration for service %v: %w", serviceConfig.Service, err) | |
| } |
## Summary This PR implements rev share feature for supplier rewards. * Adds a `Supplier.RevShare` slice property. * Adds the corresponding supplier staking configuration parser along with its tests. * Updates the tests to include the mandatory `Supplier.RevShare` property. * Includes revshare testing in the tokenomics test suite. * Updates the supplier staking config documentation _Note: ~1100LOC are protobuf autogenerated code._ ## Issue - #496 ## Type of change Select one or more: - [x] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing **Documentation changes** (only if making doc changes) - [x] `make docusaurus_start`; only needed if you make doc changes **Local Testing** (only if making code changes) - [x] **Unit Tests**: `make go_develop_and_test` - [x] **LocalNet E2E Tests**: `make test_e2e` - See [quickstart guide](https://dev.poktroll.com/developer_guide/quickstart) for instructions **PR Testing** (only if making code changes) - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. - **THIS IS VERY EXPENSIVE**, so only do it after all the reviews are complete. - Optionally run `make trigger_ci` if you want to re-trigger tests without any code changes - If tests fail, try re-running failed tests only using the GitHub UI as shown [here](https://github.com/pokt-network/poktroll/assets/1892194/607984e9-0615-4569-9452-4c730190c1d2) ## Sanity Checklist - [x] I have tested my changes using the available tooling - [x] I have commented my code - [x] I have performed a self-review of my own code; both comments & source code - [ ] I create and reference any new tickets, if applicable - [ ] I have left TODOs throughout the codebase, if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced flexible revenue sharing configurations for suppliers, allowing for detailed management of service revenue shares. - Added new fields to configuration files and structures to support default and service-specific revenue share percentages. - Implemented stricter validation for revenue share entries to ensure compliance with defined criteria. - **Bug Fixes** - Enhanced error handling related to invalid revenue shares and owner addresses in configurations. - **Tests** - Expanded test coverage to include revenue sharing logic, ensuring robust validation of configurations and expected behaviors. - Added new test cases for various scenarios, including configurations with default and service-specific revenue shares. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Daniel Olshansky <[email protected]>
Summary
This PR implements rev share feature for supplier rewards.
Supplier.RevShareslice property.Supplier.RevShareproperty.Note: ~1100LOC are protobuf autogenerated code.
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests