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

added function to unregister host zone #1166

Merged
merged 4 commits into from
Mar 27, 2024
Merged

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Mar 26, 2024

Context

Added helper function (not exposed via txs) to unregister a host zone. This will be used only on our testnet

Brief Changelog

  • Added function to undo the commands during host zone registration. This includes:
    • Removing the host zones struct
    • Removing the module accounts
    • Removing the deposit and epoch unbonding records
    • Removing the whitelisted addresses pairs

Testing

To test, I added a condition to the register host zone function (for testing purposes only) that would un-register the host zone if the function was ran with a host that already exists. Then I tested with the following:

  • Updated start_relayers to register a second set of clients/connections/channels for GAIA
  • Started dockernet
make start-docker
  • Confirmed the host zone was registered
build/strided --home dockernet/state/stride1 q stakeibc list-host-zone
  • Called the register-host-zone command - which since the host already exists, should un-register it
build/strided --home dockernet/state/stride1 tx stakeibc register-host-zone \
    connection-0 uatom cosmos \
    ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2 \
    channel-0 1 true \
    --from admin -y --gas 4000000
  • Confirmed the host zone was removed
build/strided --home dockernet/state/stride1 q stakeibc list-host-zone
  • Ran the registration command again to re-register with a different connection ID
build/strided --home dockernet/state/stride1 tx stakeibc register-host-zone \
    connection-1 uatom cosmos \
    ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2 \
    channel-1 1 true \
    --from admin -y --gas 4000000
  • Finally, confirmed the host zone was registered again
build/strided --home dockernet/state/stride1 q stakeibc list-host-zone

Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

This looks good to me, although how is this function meant to be called on testnet? Is it through governance?

Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM

Do we want to add this behind an admin gated tx?

Should we do similar ones for staketia and stakedym which have different fields and record types?

x/stakeibc/keeper/host_zone.go Show resolved Hide resolved
@sampocs
Copy link
Collaborator Author

sampocs commented Mar 27, 2024

@shellvish

This looks good to me, although how is this function meant to be called on testnet? Is it through governance?

Just through the upgrade handler! Considering it's unlikely to be used outside of the testnet, I don't think we should make a tx for it

@shellvish
Copy link
Contributor

Just through the upgrade handler! Considering it's unlikely to be used outside of the testnet, I don't think we should make a tx for it

That's perfect @sampocs! Just want to make sure there's no worry of this accidentally being called where it shouldn't be. I'm signed off on this!

Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

Approved based on comments!

@sampocs sampocs merged commit ed8a8a5 into main Mar 27, 2024
10 checks passed
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.

3 participants