[Supplier] Implement non-custodial staking #716
Conversation
red-0ne
added
supplier
This should be feature complete but don't want to mark it as ready for review before ensuring that all tests are passing. |
|
@red-0ne You mentioned this is feature complete, but how about the detailed CLI helpers we discussed? This is a very big painpoint in Morse, and the docs there are currently way ahead. See screenshot below.
|
|
Warning Rate limit exceeded@red-0ne has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 29 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the management of supplier ownership and staking by introducing a new Changes
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 (
|
proto/poktroll/shared/supplier.proto
Outdated
| string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // Bech32 cosmos address | ||
| // The operator address of the supplier that operates it. | ||
| // The operator address can update the supplier's configurations excluding the owner | ||
| // and operator addresses which do not change over the supplier's lifespan. |
There was a problem hiding this comment.
and operator addresses which do not change over the supplier's lifespan.
This is incorrect.
The operator can:
- Update operator address
The owner can:
- Update operator address
- Update owner address.
There was a problem hiding this comment.
I have added this restriction to simplify the Supplier management.
Given that:
-
Changing the operator address mid-session would have the same effect as changing other service configs, which raises the need to delay the update til the next session.
-
The
OperatorAddressis theSupplieridentifier, if the owner is allowed to change it, it would need to specify the oldOperatorAddressso we would know which supplier to update when callingGetSupplier(we actually have to delete then set theSupplierentry in theSupplierKVStore). Adding anold_operator_addressto the staking config file is far from ideal ux wise.
Having the owner being able to update the OwnerAddress is fine though.
There was a problem hiding this comment.
Having the owner being able to update the OwnerAddress is fine though.
Let's make this clearer.
There was a problem hiding this comment.
Changing the operator address mid-session would have the same effect as changing other service configs, which raises the need to delay the update til the next session.
Add one line making it clearer why operator address is immutable.
There was a problem hiding this comment.
Per this comment: #716 (comment)
Can you please evaluate if there is any difference with how it works in Morse?
There was a problem hiding this comment.
Looking directly into the code, these are the current differences.
| Morse | Shannon | |
|---|---|---|
| Id | Operator address | Operator address |
| Initial stake | Owner and Operator | Owner only |
| Update | Owner and operator | Operator only |
| Update owner | Owner | None, must unstake->stake |
| Update operator | Owner and Operator (see Note) | None, must unstake->stake |
| Unstake | Owner and Operator | Owner only (can easily upgrade to both) |
Note that from reading the code, ther seems to be no code path that can update the operator address, since the message would contain a new validator address which would not be found when doing k.GetValidator(ctx, validator.Address). I may be missing something though
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (4)
x/supplier/module/tx_unstake_supplier.go (1)
15-15: Clarify the command usage.The usage string now requires an
<operator_address>, which is a necessary parameter for unstaking. Ensure that this requirement is clearly documented in the CLI help and related documentation.proto/poktroll/shared/supplier.proto (2)
13-16: Clarify the owner address documentation.The documentation for
owner_addressis clear, specifying its role and immutability by the operator. Ensure this is consistently communicated in related documentation.
17-23: Clarify the operator address documentation.The
addressfield now represents the operator address. The comments clarify its role and immutability. Ensure this is reflected in all related documentation, and address the TODO for renaming.testutil/integration/app.go (1)
425-425: Update all calls toNewCompleteIntegrationAppto includesupplierKeeper.The function
NewCompleteIntegrationApphas been updated to includesupplierKeeperin its signature, but the following calls do not pass this argument:
tests/integration/tokenomics/tokenomics_example_test.gotests/integration/tokenomics/relay_mining_difficulty_test.goPlease ensure these calls are updated to match the new function signature.
Analysis chain
Update function signature to include
supplierKeeper.The addition of
supplierKeeperto the function signature ofNewCompleteIntegrationAppindicates a change in the initialization process of the integration app. Ensure that all calls to this function are updated to pass thesupplierKeeperargument.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewCompleteIntegrationApp` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 3 $'NewCompleteIntegrationApp'Length of output: 1923
| signerAddress string, | ||
| ownerAddress string, | ||
| supplierAddress string, | ||
| stake sdk.Coin, | ||
| services []*sharedtypes.SupplierServiceConfig, | ||
| ) *MsgStakeSupplier { | ||
| return &MsgStakeSupplier{ | ||
| Address: address, | ||
| Stake: &stake, | ||
| Services: services, | ||
| Signer: signerAddress, | ||
| OwnerAddress: ownerAddress, | ||
| Address: supplierAddress, | ||
| Stake: &stake, | ||
| Services: services, |
There was a problem hiding this comment.
Ensure non-empty parameters in NewMsgStakeSupplier.
The function should validate that signerAddress, ownerAddress, and supplierAddress are non-empty strings to prevent creating invalid messages.
if signerAddress == "" || ownerAddress == "" || supplierAddress == "" {
return nil, errors.New("addresses must be non-empty")
}
| @@ -31,3 +31,13 @@ func (s *Supplier) IsActive(queryHeight uint64, serviceId string) bool { | |||
|
|
|||
| return true | |||
| } | |||
|
|
|||
| // HasOwner returns whether the given address is the supplier's owner address. | |||
| func (s *Supplier) HasOwner(address string) bool { | |||
| coinsToEscrow = *msg.Stake | ||
| } else { | ||
| logger.Info(fmt.Sprintf("Supplier found. About to try updating supplier with address %q", msg.Address)) | ||
|
|
||
| // Ensure the signer is either the owner or the operator of the supplier. |
| ) | ||
| } | ||
|
|
||
| // Ensure that only the owner can change the OwnerAddress. |
| @@ -12,22 +12,28 @@ import ( | |||
| func CmdUnstakeSupplier() *cobra.Command { | |||
There was a problem hiding this comment.
Not actionable: Just a reminder that we'll need to use AutoCLI in the future.
| // address is the must be the owner or operator address of the supplier | ||
| address := args[0] | ||
|
|
||
| signerAddress := clientCtx.GetFromAddress().String() |
|
|
||
| string signer = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the message signer (i.e. owner or operator) | ||
| string owner_address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the owner (i.e. custodial, staker) | ||
| string address = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the operator (i.e. provider, non-custodial) |
There was a problem hiding this comment.
Should this be operator_address or have a TODO_TECHDEBT(#722) comment?
There was a problem hiding this comment.
Is already renamed in #722
| option (cosmos.msg.v1.signer) = "signer"; | ||
|
|
||
| string signer = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the message signer (i.e. owner or operator) | ||
| string address = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the operator (i.e. provider, non-custodial) |
| OwnerAddress: ownerAddress, | ||
| Address: supplierAddress, | ||
| Stake: &stake, | ||
| Services: services, | ||
| } | ||
| } | ||
|
|
||
| func (msg *MsgStakeSupplier) ValidateBasic() error { |
There was a problem hiding this comment.
Does it make sense to move the check which ensures that the signer is either the owner or operator here instead?
There was a problem hiding this comment.
We can't, since we may have an external signer (neither the operator, nor the owner) that can stake the funds.
See: https://discord.com/channels/553741558869131266/1266860790917632121/1271475296805322762
| return &MsgUnstakeSupplier{ | ||
| Signer: signerAddress, | ||
| Address: address, | ||
| } | ||
| } | ||
|
|
||
| func (msg *MsgUnstakeSupplier) ValidateBasic() error { |
There was a problem hiding this comment.
When unstaking, we check that the signer is either the supplier's owner or operator, which is not available in ValidateBasic. The Address here just indicate which operator to unstake.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
x/supplier/config/supplier_configs_reader.go (1)
84-85: Consider logging the defaulting of the operator address.Adding a log statement when the operator address defaults to the owner address can help with debugging and tracking configuration changes.
- logger.Info().Msg("operator address not set, defaulting to owner address") + logger.Info().Str("operator_address", stakeConfig.OperatorAddress).Msg("operator address not set, defaulting to owner address")
## Summary This PR is a follow-up to #716. * It refactors the existing unit/integration tests to add `Supplier.OwnerAddress` * Adapts the `Supplier` (un)staking and tests (staking config, (un)staking CLI and keeper methods) to comply with the `Supplier` non-custodial staking. ## Issue - #493 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing **Documentation changes** (only if making doc changes) - [ ] `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` - [ ] **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 - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] 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 - **New Features** - Enhanced readability of output messages in the Makefile. - Added `OwnerAddress` to the `Supplier` data model for better ownership tracking. - Introduced support for both owner and operator addresses in various staking and unstaking processes. - **Bug Fixes** - Updated tests for `MsgUnstakeSupplier` to correctly validate signer addresses and enhance robustness. - **Documentation** - Improved test coverage and clarity for supplier-related functionality, reflecting the new data model changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Follow-up to #716 to document custodial and non-custodial staking configuration. ## Issue  - #493 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [x] 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) - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **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 - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] 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 - **New Features** - Enhanced command for supplier unstake, improving usability and flexibility. - Introduced new sections in documentation for "Staking types," detailing Custodial and Non-Custodial Staking. - **Documentation** - Expanded configuration documentation with detailed explanations of `owner_address` and `operator_address`. - **Config Updates** - Added `owner_address` and `operator_address` parameters to multiple staking configuration files for improved clarity. - **Improvements** - Streamlined configuration files by removing comments, enhancing clarity on staking roles. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary This PR implements non-custodial staking by * Adding a `Supplier.OwnerAddress` that represents the `Supplier` owner * Modifying `stake` and `unstake` CLI * Ensure that only the owner is able to `stake`, `unstake`, `change owner`, `change operator` * Update supplier staking config parser * Have the owner receive the rewards. *NOTE: ~950LOC is proto auto-generated code* The PR will be ready after: - [ ] Validating non-custodial logic added in this PR - [ ] Merging #718 into this one - [ ] Merging #720 into this one - [ ] Merging #722 into this one ## Issue - #493 - https://www.notion.so/buildwithgrove/Non-custodial-staking-92a136174dac41279717e8b963672e38 ## 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) - [ ] `make docusaurus_start`; only needed if you make doc changes **Local Testing** (only if making code changes) - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **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 - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] 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 - **New Features** - Introduced a new field `OwnerAddress` in the `Supplier` struct for improved ownership management. - Enhanced supplier configuration structures with `OwnerAddress` and `OperatorAddress` fields for better role management. - Added new validation methods to ensure correct ownership and operator address integrity. - **Bug Fixes** - Improved validation logic to ensure only authorized users can modify supplier details. - **Documentation** - Clarified comments in code to improve understanding of address management and supplier roles. - **Chores** - Added new error constants to enhance error handling and user feedback for supplier-related operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary This PR is a follow-up to #716. * It refactors the existing unit/integration tests to add `Supplier.OwnerAddress` * Adapts the `Supplier` (un)staking and tests (staking config, (un)staking CLI and keeper methods) to comply with the `Supplier` non-custodial staking. ## Issue - #493 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing **Documentation changes** (only if making doc changes) - [ ] `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` - [ ] **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 - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] 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 - **New Features** - Enhanced readability of output messages in the Makefile. - Added `OwnerAddress` to the `Supplier` data model for better ownership tracking. - Introduced support for both owner and operator addresses in various staking and unstaking processes. - **Bug Fixes** - Updated tests for `MsgUnstakeSupplier` to correctly validate signer addresses and enhance robustness. - **Documentation** - Improved test coverage and clarity for supplier-related functionality, reflecting the new data model changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Follow-up to #716 to document custodial and non-custodial staking configuration. ## Issue  - #493 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [x] 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) - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **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 - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] 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 - **New Features** - Enhanced command for supplier unstake, improving usability and flexibility. - Introduced new sections in documentation for "Staking types," detailing Custodial and Non-Custodial Staking. - **Documentation** - Expanded configuration documentation with detailed explanations of `owner_address` and `operator_address`. - **Config Updates** - Added `owner_address` and `operator_address` parameters to multiple staking configuration files for improved clarity. - **Improvements** - Streamlined configuration files by removing comments, enhancing clarity on staking roles. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
This PR implements non-custodial staking by
Supplier.OwnerAddressthat represents theSupplierownerstakeandunstakeCLIstake,unstake,change owner,change operatorNOTE: ~950LOC is proto auto-generated code
The PR will be ready after:
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
OwnerAddressin theSupplierstruct for improved ownership management.OwnerAddressandOperatorAddressfields for better role management.Bug Fixes
Documentation
Chores