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

Move disable tokenization to hooks #1066

Merged
merged 17 commits into from
Jan 11, 2024
Merged

Move disable tokenization to hooks #1066

merged 17 commits into from
Jan 11, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Jan 11, 2024

Context

Submitting the detokenization ICA from the upgrade handler doesn't seem trivial. Instead, we're temporarily moving it to hooks and will remove in the next upgrade.

Brief Changelog

  • Moved disable tokenization function to hooks, running once every 10 days
  • Fixed/moved unit tests

Testing Strategy

  • Changed cosmoshub-4 chain-id to GAIA in the code and changed it to run every day epoch (instead of every 10)
  • Started dockernet on the old binary (wasn't totally necessary)
  • Checked that the delegation ICA was locked
>>> gaiadl q staking tokenize-share-lock-info cosmos1eld82php5h2dpgke8ansn5pajrgu69pzkpajn0mkuf3ujk6lvz9qyvdd48
expiration_time: ""
status: TOKENIZE_SHARE_LOCK_STATUS_UNLOCKED
  • Ran through the upgrade
  • Waited for an epoch
  • Confirmed the account was locked
>>> gaiadl q staking tokenize-share-lock-info cosmos1eld82php5h2dpgke8ansn5pajrgu69pzkpajn0mkuf3ujk6lvz9qyvdd48
expiration_time: ""
status: TOKENIZE_SHARE_LOCK_STATUS_LOCKED
  • Confirmed there were ack errors after the first one succeeded (since you can only lock once), but that the failures were just ignored and the account stayed locked

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Is there a way to query whether an acct has tokenization disabled? If so we should test this with a quick script in dockernet.

@riley-stride
Copy link
Contributor

Found it. We can query q staking tokenize-share-lock-info <address>. Can we write a quick script to test this?

@sampocs
Copy link
Collaborator Author

sampocs commented Jan 11, 2024

yeah I'm testing at the moment

dockernet/config/ica_host.json Show resolved Hide resolved
x/stakeibc/keeper/hooks.go Outdated Show resolved Hide resolved
@sampocs
Copy link
Collaborator Author

sampocs commented Jan 11, 2024

@riley-stride @asalzmann Successfully tested on dockernet - added testing steps to PR description

Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

lgtm pending the .gitmodules / dockerfile changes

.gitmodules Show resolved Hide resolved
dockernet/dockerfiles/Dockerfile.dydx Show resolved Hide resolved
@sampocs sampocs added the A:automerge Automatically merge PR once checks pass label Jan 11, 2024
@mergify mergify bot merged commit 4674eed into main Jan 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once checks pass C:app-wiring C:stakeibc C:submodules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants