Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Supplier] Rename Supplier.Address to Supplier.OperatorAddress #722

Merged
merged 11 commits into from
Aug 13, 2024
Merged

[Supplier] Rename Supplier.Address to Supplier.OperatorAddress #722

merged 11 commits into from
Aug 13, 2024

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Aug 6, 2024 •
edited
Loading

Summary

This PR renames all the Supplier.Address references to Supplier.OperatorAddress.

It also include renaming supplierAddress to supplierOperatorAddress variables, methods in Claim and Proof, comments and error messages.

This PR has a lot of changes but does not introduce any addition logic to the code base.

Issue

Type of change

Select one or more:

  • 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 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

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

@red-0ne red-0ne requested a review from Olshansk August 6, 2024 15:08
@red-0ne red-0ne self-assigned this Aug 6, 2024
@red-0ne red-0ne added supplier Changes related to the Supplier actor code health Cleans up some code labels Aug 6, 2024
@red-0ne red-0ne changed the base branch from docs/non-custodial-staking to refactor/non-custodial-staking-tests August 6, 2024 15:12
@red-0ne red-0ne mentioned this pull request Aug 6, 2024
Merged
18 tasks
@Olshansk Olshansk added this to the Shannon Beta TestNet Launch milestone Aug 7, 2024
Olshansk
Olshansk requested changes Aug 7, 2024
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not review everything, but I did a very simple ctrl-f all (left some comments) and took a look at the .proto files.

No issues, but just please do one more sanity check (e.g. update docs per my suggested search)

pkg/client/interface.go Show resolved Hide resolved
docusaurus/docs/protocol/primitives/claim_and_proof_lifecycle.md Show resolved Hide resolved
x/proof/module/query_claim.go Show resolved Hide resolved
proto/poktroll/proof/tx.proto Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk August 9, 2024 08:00
Olshansk
Olshansk approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's merge this into main AFTER the others were merged as well.

pkg/client/supplier/options.go Outdated Show resolved Hide resolved
pkg/deps/config/suppliers.go Outdated Show resolved Hide resolved
red-0ne added a commit that referenced this pull request Aug 13, 2024
@red-0ne
[Supplier] Implement non-custodial staking (#716)
d296b17 
## 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 -->
@github-actions GitHub Actions
Copy link

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 make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 722)
Grafana network dashboard for devnet-issue-722

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Aug 13, 2024
@red-0ne red-0ne merged commit 0625c67 into refactor/non-custodial-staking-tests Aug 13, 2024
4 checks passed
@red-0ne red-0ne mentioned this pull request Aug 13, 2024
Merged
14 tasks
red-0ne added a commit that referenced this pull request Aug 14, 2024
@red-0ne
[Supplier] Rename Supplier.Address to Supplier.OperatorAddress (#734)
5a10fba 
## Summary

*This PR is duplicate of #722 which got mistakingly merged into
`refactor/non-custodial-staking-tests`*

This PR renames all the Supplier.Address references to
Supplier.OperatorAddress.

It also include renaming supplierAddress to supplierOperatorAddress
variables, methods in Claim and Proof, comments and error messages.

This PR has a lot of changes but does not introduce any addition logic
to the code base.

## 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`
- [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
- [ ] 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
okdas pushed a commit that referenced this pull request Nov 14, 2024
@red-0ne
[Supplier] Implement non-custodial staking (#716)
a50222a 
## 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 -->
okdas pushed a commit that referenced this pull request Nov 14, 2024
@red-0ne
[Supplier] Rename Supplier.Address to Supplier.OperatorAddress (#734)
bdb9f95 
## Summary

*This PR is duplicate of #722 which got mistakingly merged into
`refactor/non-custodial-staking-tests`*

This PR renames all the Supplier.Address references to
Supplier.OperatorAddress.

It also include renaming supplierAddress to supplierOperatorAddress
variables, methods in Claim and Proof, comments and error messages.

This PR has a lot of changes but does not introduce any addition logic
to the code base.

## 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`
- [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
- [ ] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk approved these changes

Assignees

@red-0ne red-0ne

Labels
code health Cleans up some code devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io supplier Changes related to the Supplier actor
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Beta TestNet Launch
Development

Successfully merging this pull request may close these issues.
2 participants
@red-0ne @Olshansk