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.

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

[Gateway] Add min_stake gateway module param #809

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Sep 10, 2024

Summary

Adds the min_stake_gateway param to the gateway.

Depends on

Issue

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 bryanchriswhite added gateway Changes related to the Gateway actor governance Governance related changes on-chain On-chain business logic consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. labels Sep 10, 2024
@bryanchriswhite bryanchriswhite self-assigned this Sep 10, 2024
@bryanchriswhite bryanchriswhite linked an issue Sep 10, 2024 that may be closed by this pull request
10 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/612/chore/gateway-msg-update-param branch from 65e0659 to 59b9b2d Compare September 23, 2024 15:40
@bryanchriswhite bryanchriswhite force-pushed the issues/612/chore/gateway-msg-update-param branch from 59b9b2d to ae16cf3 Compare September 23, 2024 19:36
@bryanchriswhite bryanchriswhite force-pushed the issues/612/param/min-stake-gateway branch 2 times, most recently from 6783b88 to 7e66b1d Compare September 23, 2024 19:49
bryanchriswhite added a commit that referenced this pull request Sep 25, 2024
## Summary

Ports the `MsgUpdateParams` and `MsgUpdateParam` E2E tests to
integration tests, improving execution speed and maintainability.

## Depends on

- #827 
- #820 
- #821
- #815

## Dependents

- #809 

## Issue

- #799

## Type of change

Select one or more from the following:

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

## Testing

- [x] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [ ] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [x] **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
- [x] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
@bryanchriswhite bryanchriswhite force-pushed the issues/612/chore/gateway-msg-update-param branch from ae16cf3 to f7a6e5b Compare September 26, 2024 06:59
@bryanchriswhite bryanchriswhite force-pushed the issues/612/param/min-stake-gateway branch 3 times, most recently from e4cc6df to 7b9c71a Compare September 26, 2024 08:37
@bryanchriswhite bryanchriswhite marked this pull request as draft September 26, 2024 11:38
@bryanchriswhite bryanchriswhite marked this pull request as ready for review September 26, 2024 13:52
@bryanchriswhite bryanchriswhite changed the title [Params] Add min_stake_gateway gateway module param [Params] Add min_stake gateway module param Sep 27, 2024
@bryanchriswhite bryanchriswhite changed the title [Params] Add min_stake gateway module param [Gateway] Add min_stake gateway module param Sep 27, 2024
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Left a few change requests but looks good overall.

config.yml Outdated Show resolved Hide resolved
tools/scripts/params/gateway_min_stake.json Outdated Show resolved Hide resolved

// Perform a global validation on all params, which includes the updated param.
// This is needed to ensure that the updated param is valid in the context of all other params.
if err := params.Validate(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err := params.Validate(); err != nil {
if err := params.ValidateBasic(); err != nil {

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 chose not to rename it this time. My rationale is that it requires changing it in lots of other places which is not conducive to making the "adding parameters" step-by-step instructions any more concise. 😅

Do you have a strong opinion on this and/or a rationale that you could share?

x/gateway/keeper/params_test.go Outdated Show resolved Hide resolved
expectedErr: gatewaytypes.ErrGatewayParamInvalid.Wrapf("invalid type for %s: int64; expected *cosmostypes.Coin", gatewaytypes.ParamMinStake),
},
{
desc: "MinStake less than zero",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we validate that it's greater than zero?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Sep 27, 2024

Choose a reason for hiding this comment

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

Yes; however, the test descriptions are describing the test case. In this case, the min stake is less than zero. I was aiming for consistency with other tests but we can change it (and document it).

Do you have a principled way to think about how we should be phrasing test cases / scenarios?

bryanchriswhite and others added 4 commits September 30, 2024 11:12
Co-authored-by: Redouane Lakrache <[email protected]>
…e-param' into issues/612/param/min-stake-gateway

* pokt/issues/612/chore/gateway-msg-update-param:
  chore: review feedback improvements
  [Docs] Update README(s) (#842)
bryanchriswhite added a commit that referenced this pull request Sep 30, 2024
## Summary

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

Adds the `MsgUpdateParam` message so that the gateway module may update
individual parameters. The gateway module's `min_stake` param will be
added in #809.

## 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]>
@bryanchriswhite bryanchriswhite changed the base branch from issues/612/chore/gateway-msg-update-param to main September 30, 2024 10:48
…ake-gateway

* pokt/main:
  [Gateway] chore: add `MsgUpdateParam` to gateway module (#808)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. gateway Changes related to the Gateway actor governance Governance related changes on-chain On-chain business logic
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[Utility] Minimum staking values for each actor
2 participants