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

[Gateway] Enforce minimum stake when staking #843

Merged
merged 53 commits into from
Oct 4, 2024
Merged

[Gateway] Enforce minimum stake when staking #843

merged 53 commits into from
Oct 4, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Sep 27, 2024 •
edited
Loading

Summary

  1. Adds minimum stake validation to the gateway stake message handler (i.e. total stake must be >= minimum stake).
  2. Updates error returns in the same handler to ensure all error paths return appropriate gRPC status errors.
  3. Replaces some warn and error level logs with info level, which I believe is more appropriate (until we have a practical debug level, at which point they should become debug logs).

Dependencies

Dependents

Issue

Gateway staking min stake validation.

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

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

@bryanchriswhite
scaffold: message update-param --module gateway --signer authority na…
d1cd5e6 
…me as_type --response params
@bryanchriswhite
chore: update gateway MsgUpdateParam fields
91eecfc
@bryanchriswhite
chore: update MsgUpdateParamResponse fields
ef62280
@bryanchriswhite
chore: comment out autocli for gateway module param updates
f7a6e5b
@bryanchriswhite
fix: linter error
8957f1c
@bryanchriswhite
revert: premature changes
3016ca3
@bryanchriswhite
feat: add min_stake gateway module param
899effb
@bryanchriswhite
chore: cleanup makefiles and param update JSON files
83c1d74
@bryanchriswhite
chore: add gateway module param make targets
55e6a31
@bryanchriswhite
refctor: consolidate param config types
ddd68bb
@bryanchriswhite
tests: improve error messaging
fa8caf6
@bryanchriswhite
tests: add coverage over gateway min stake param updates
ae006e2
@bryanchriswhite
chore: add as_coin as_type type 😉
e92e3b1
@bryanchriswhite
chore: add app MsgUpdateParam to genesis authorizations
b2e1267
@bryanchriswhite
fix: typo
0373fa2
@bryanchriswhite
chore: cleanup makefiles
da7894a
@bryanchriswhite
wip: fixing tests
015c537
@bryanchriswhite
tests: add tests
1ac6c48
@bryanchriswhite
tests: remove erroneous case
db366f6
@bryanchriswhite
test: gateway staking below minimum fails
60bef17
@bryanchriswhite
feat: gateway min stake validation & grpc status error returns
a21687f
@bryanchriswhite
chore: cleanup comments
7e9f857
@bryanchriswhite bryanchriswhite added gateway Changes related to the Gateway actor protocol General core protocol related changes consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. labels Sep 27, 2024
@bryanchriswhite bryanchriswhite self-assigned this Sep 27, 2024
@bryanchriswhite bryanchriswhite marked this pull request as ready for review September 27, 2024 08:19
@bryanchriswhite bryanchriswhite changed the base branch from issues/612/param/min-stake-gateway to main October 4, 2024 11:54
Olshansk
Olshansk approved these changes Oct 4, 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.

Small comment add but LGTM otherwise.

x/gateway/keeper/msg_server_stake_gateway.go Show resolved Hide resolved
Olshansk
Olshansk approved these changes Oct 4, 2024
View reviewed changes
x/gateway/keeper/msg_server_stake_gateway.go
// NB: This SHOULD NEVER happen because msg.ValidateBasic() validates the address as bech32.
if err != nil {
// TODO_TECHDEBT(#384): determine whether to continue using cosmos logger for debug level.
logger.Info(fmt.Sprintf("ERROR: could not parse address %q", msg.Address))
Copy link
Member

Choose a reason for hiding this comment

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

Calling out to revisit the logger.Info vs logger.Debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened #856

@bryanchriswhite bryanchriswhite mentioned this pull request Oct 4, 2024
Open
4 tasks
@bryanchriswhite bryanchriswhite mentioned this pull request Oct 4, 2024
Open
15 tasks
@bryanchriswhite bryanchriswhite linked an issue Oct 4, 2024 that may be closed by this pull request
[Utility] Minimum staking values for each actor #612
Open
10 tasks
@bryanchriswhite bryanchriswhite merged commit 12a7578 into main Oct 4, 2024
10 checks passed
bryanchriswhite added a commit that referenced this pull request Oct 4, 2024
@bryanchriswhite @red-0ne @Olshansk
[Application] chore: add MsgUpdateParam to application module (#844)
bb0ec85 
## Summary

```bash
ignite scaffold message update-param --module gateway --signer authority name as_type --response params
```

Adds the `MsgUpdateParam` message so that the application module may
update individual parameters. The application module's min_stake param
will be added in a subsequent PR.

## Dependencies

- #809
- #843 

## Dependents

- #845 
- #847 
- #848 
- #849
- #850
- #857

## Issue

Add individual param updates support to the application module

- #612

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [ ] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

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

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
bryanchriswhite added a commit that referenced this pull request Oct 4, 2024
@bryanchriswhite @red-0ne @Olshansk
[Application] Add min_stake application module param (#845)
49290e3 
## Summary

- Adds the `min_stake` application module param.
- Adds missing validation unit test coverage around existing application
module param, `max_delegated_gateways`.
- Pushes validation from the `MsgUpdateParam` handler down into
`MsgUpdateParam#ValidateBasic()`.
- Ensures `Params#Valid()` is called prior to setting params in
`MsgUpdateParam` handler.
- Updates error returns in the same handler to ensure all error paths
return appropriate gRPC status errors.

## Dependencies

- #809
- #843 
- #844 

## Dependents

- #847 
- #848 
- #849
- #850
- #857

## Issue

- #612

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [ ] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

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

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
bryanchriswhite added a commit that referenced this pull request Oct 7, 2024
@bryanchriswhite
Merge branch 'issues/612/application/staking' into issues/612/applica…
5e5d8a4 
…tion/burning

* issues/612/application/staking:
  [Application] Add `min_stake` application module param (#845)
  chore: regenerate protobufs
  [Application] chore: add `MsgUpdateParam` to application module  (#844)
  fix: linter error
  [Gateway] Enforce minimum stake when staking (#843)
  Empty commit
  [Tokenomics] Use big.Rat to calculate new difficulty hash (#831)
  chore: review feedback improvements
  Empty commit
  fix: linter errors
  fix: typo
  chore: review feedback improvements
This was referenced Oct 9, 2024
Open
Open
Open
bryanchriswhite added a commit that referenced this pull request Oct 9, 2024
@bryanchriswhite @red-0ne @Olshansk
[Application] Enforce minimum stake when staking (#847)
fd71973 
## Summary

- Adds minimum stake validation to the application stake message handler
(i.e. total stake must be >= minimum stake).
- Updates error returns in the same handler to ensure all error paths
return appropriate gRPC status errors.
- Replaces some warn and error level logs with info level, which I
believe is more appropriate (until we have a practical debug level, at
which point they should become debug logs).

## Dependencies

- #809
- #843 
- #844 
- #845

## Dependents
 
- #848 
- #849
- #850
- #857
- #852 
- #861 
- #851 
- #863 

## Issue

- #612

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [ ] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] 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
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
bryanchriswhite added a commit that referenced this pull request Oct 11, 2024
@bryanchriswhite @red-0ne @Olshansk
[Application] Enforce minimum stake when burning (#848)
b60dac9 
## Summary

Updates `ProcessTokenLogicModules()` logic to unbond applications whose
stake is below the minimum **after** processing all TLMs.

## Dependencies

- #809
- #843 
- #844 
- #845
- #847

## Dependents
 
- #849
- #850
- #857
- #852 
- #861 
- #851 
- #863 

## Issue

- #612

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## 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
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@red-0ne red-0ne red-0ne approved these changes

@Olshansk Olshansk Olshansk approved these changes

Assignees

@bryanchriswhite bryanchriswhite

Labels
consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. devnet devnet-test-e2e gateway Changes related to the Gateway actor on-chain On-chain business logic protocol General core protocol related changes push-image CI related - pushes images to ghcr.io
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Beta TestNet Launch
Development

Successfully merging this pull request may close these issues.

[Utility] Minimum staking values for each actor
3 participants
@bryanchriswhite @red-0ne @Olshansk