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

Add Multiple Validators in One Transaction #677

Merged
merged 7 commits into from
Mar 24, 2023
Merged

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Mar 22, 2023

Closes: #XXX

Context and purpose of the change

The add-validator function was previous limited to adding only one validator at a time. This PR alters the transaction so that a JSON with an arbitrary number of validators can be supplied to improve the dev experience.
NOTE FOR REVIEWER: It might be easier to scroll through commits since files were renamed

Brief Changelog

  • Change add-validatoradd-validators
  • Modify add-validators to take a list of validators as a JSON instead of a single validator
  • Removed unused fields from validator struct. We should confirm with localstride that this doesn't require a migration.
    • I did test by running the old binary, and then querying the host zones with this binary. The removed fields did not appear and the query did not error - indicating that this new binary can successfully deserialize the old store (which should mean we do not need a state migration).

Testing

Setup

  • Start dockernet
git submodule update --init --recursive
make start-docker build=sgr
  • Confirm there's one validator
strided q stakeibc list-host-zone

Success Case

  • Create a json file called add_validator_admin.json as follows:
{
    "validators": [
        {"name": "val-from-admin-1", "address": "cosmosvaloper196ax4vc0lwpxndu9dyhvca7jhxp70rmcvrj90c", "weight": 10},
        {"name": "val-from-admin-2", "address": "cosmosvaloper1v5y0tg0jllvxf5c3afml8s3awue0ymju89frut", "weight": 0},
        {"name": "val-from-admin-3", "address": "cosmosvaloper1clpqr4nrk4khgkxj78fcwwh6dl3uw4epsluffn", "weight": 30}
    ]
}
  • Create json file called add_validator_gov.json as follows:
{
    "description": "Proposal to add GAIA validators",
    "hostZone": "GAIA",
    "validators": [
        {"name": "val-from-gov-1", "address": "cosmosvaloper1lzhlnpahvznwfv4jmay2tgaha5kmz5qxerarrl"},
        {"name": "val-from-gov-2", "address": "cosmosvaloper16k579jk6yt2cwmqx9dz5xvq9fug2tekvlu9qdv"},
        {"name": "val-from-gov-3", "address": "cosmosvaloper1g48268mu5vfp4wk7dk89r0wdrakm9p5xk0q50k"}
     ],
    "deposit": "64000000ustrd"
}
  • Add validators through admin function
strided tx stakeibc add-validators GAIA add_validator_admin.json --from admin 
  • Confirm admin validators were added successfully
strided q stakeibc list-host-zone
  • Add validators though governance (you have 30 seconds to get the votes in!)
strided tx gov submit-legacy-proposal add-validators add_validator_gov.json --from val1 --gas auto -y 
strided tx gov vote 1 yes --from val1 -y
strided tx gov vote 1 yes --from val2 -y
strided tx gov vote 1 yes --from val3 -y
  • After the voting period has passed (30 seconds), confirm the gov validators were added. Note that the weight is equal to 5 (the lowest weight, excluding 0)
strided q stakeibc list-host-zone

Failure Case - Invalid Host Zone

  • Attempt to add validators with a host zone that does not exist, it should fail
strided tx stakeibc add-validators FAKE add_validator_admin.json --from admin 

Failure Case - Admin - Validator already exists

  • For each of the following, inspect the tx hash to identify the failure
  • Replace add_validator_admin.json with the following
{
    "validators": [
        {"name": "val-from-admin-8", "address": "cosmosvaloper14kn0kk33szpwus9nh8n87fjel8djx0y070ymmj", "weight": 10},
        {"name": "val-from-admin-1", "address": "cosmosvaloper1vvwtk805lxehwle9l4yudmq6mn0g32px9xtkhc", "weight": 10}
    ]
}
  • Attempt to add the validators - it should fail because the validator name already exists
strided tx stakeibc add-validators GAIA add_validator_admin.json --from admin 
  • Now replace it again with the following
{
    "validators": [
        {"name": "val-from-admin-8", "address": "cosmosvaloper14kn0kk33szpwus9nh8n87fjel8djx0y070ymmj", "weight": 10},
        {"name": "val-from-admin-9", "address": "cosmosvaloper196ax4vc0lwpxndu9dyhvca7jhxp70rmcvrj90c", "weight": 10}
    ]
}
  • Attempt to add the validators - it should fail because the validator address already exists
strided tx stakeibc add-validators GAIA add_validator_admin.json --from admin 

Failure Case - Gov - Validator already exists

  • For each of the following, the tx should fail immediately. Note: The error log is very verbose, but you can see the relevant error message if you look close enough
  • Replace add_validator_gov.json with the following
{
    "description": "Proposal to add GAIA validators",
    "hostZone": "GAIA",
    "validators": [
        {"name": "val-from-admin-8", "address": "cosmosvaloper14kn0kk33szpwus9nh8n87fjel8djx0y070ymmj", "weight": 10},
        {"name": "val-from-admin-1", "address": "cosmosvaloper1vvwtk805lxehwle9l4yudmq6mn0g32px9xtkhc", "weight": 10}
    ],
    "deposit": "64000000ustrd"
}
  • Attempt to add the validators - it should fail because the validator name already exists
strided tx gov submit-legacy-proposal add-validators add_validator_gov.json --from val1 --gas auto
  • Now replace it again with the following
{
    "description": "Proposal to add GAIA validators",
    "hostZone": "GAIA",
    "validators": [
        {"name": "val-from-admin-8", "address": "cosmosvaloper14kn0kk33szpwus9nh8n87fjel8djx0y070ymmj", "weight": 10},
        {"name": "val-from-admin-9", "address": "cosmosvaloper196ax4vc0lwpxndu9dyhvca7jhxp70rmcvrj90c", "weight": 10}
    ],
    "deposit": "64000000ustrd"
}
  • Attempt to add the validators - it should fail because the validator address already exists
strided tx gov submit-legacy-proposal add-validators add_validator_gov.json --from val1 --gas auto

Author's Checklist

I have...

  • Run and PASSED locally all GAIA integration tests
  • If the change is contentful, I either:
    • Added a new unit test OR
    • Added test cases to existing unit tests
  • OR this change is a trivial rework / code cleanup without any test coverage

If skipped any of the tests above, explain.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • reviewed state machine logic
  • reviewed API design and naming
  • manually tested (if applicable)
  • confirmed the author wrote unit tests for new logic
  • reviewed documentation exists and is accurate

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md?
  • This pull request updates existing proto field values (and require a backend and frontend migration)?
  • Does this pull request change existing proto field names (and require a frontend migration)?
    How is the feature or change documented?
    • not applicable
    • jira ticket XXX
    • specification (x/<module>/spec/)
    • README.md
    • not documented

@sampocs sampocs force-pushed the sam/add-multiple-validators branch from 9054041 to 5e39cc8 Compare March 22, 2023 23:48
@sampocs sampocs changed the title Add Multiple Validators at Once Add Multiple Validators in One Transaction Mar 23, 2023
@sampocs sampocs marked this pull request as ready for review March 23, 2023 03:04
@sampocs sampocs added the v8 label Mar 23, 2023
@riley-stride riley-stride self-requested a review March 24, 2023 02:03
@riley-stride riley-stride self-requested a review March 24, 2023 02:04
@sampocs sampocs merged commit 4ce1317 into main Mar 24, 2023
sontrinh16 pushed a commit to notional-labs/stride that referenced this pull request Mar 27, 2023
Co-authored-by: riley-stride <104941670+riley-stride@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants