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

batched undelegations #1195

Merged
merged 13 commits into from
Jun 13, 2024
Merged

batched undelegations #1195

merged 13 commits into from
Jun 13, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented May 20, 2024

Context

Base PR for the batched undelegation work. This PR consists of only proto changes, but subsequent PR's will be merged into this before main.

Changelog

New Fields on HZU

Now that the StToken and NativeToken amounts are decremented in different places, I think it makes more sense to add explicit fields for each purpose. In this vein, I added the following:

NativeTokensToUnbond

  • purpose is so that if we have failed batches, we know how much we have left that needs to be unbonded
  • set right before the ICA is submitted
  • decremented in the callback

StTokensToBurn

  • purpose is to track tokens that need to be burned
  • set right before ICA is submitted
  • decremented in callback

ClaimableNativeTokens

  • purpose is to track tokens that have been claimed
  • set right before redemption sweep ICA is submitted
  • decremented during each user claim

UndelegationTxsInProgress

  • purpose is to prevent submitting a retry until we know all txs for a given epoch have been completed
  • otherwise, it would could attempt to unbond with funds that were already unbonded (happens if the ack comes back after the epoch)
  • This is incremented when each ICA is submitted and decremented in the callback

New Status Field

Additionally, we need to differentiate EUR's that are in a retry state vs those that are new unbondings. For this, I added the new status field UNBONDING_RETRY_QUEUE

Updated Callback Data

Finally, the callback data was updated to also store the number of stTokens. Given the fact that there's some precision error when breaking down a total stToken amount across validators, I think there are two options:

  • Option 1: Only store native amount in callback, handle precision error by checking if the stAmount is below some epsilon, and if, so, rounding it down to 0
  • Option 2: When building the ICA messages, capture the stToken remainder on the last validator and store it in the callback data

I opted for option 2 as it seems less error prone.

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.

looks good, ty for the detailed description

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, thank you for explaining new fields.
As the github alerts point out, this change to the protos means that this PR alone has some inconsistent types now -- like in icacallback_undelegate on line 48 it still refers to the old field called SplitDelegations instead of the new SplitUndelegation but I assume that these are fixed in the later PRs which actually update the callbacks

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.

Looks great, thanks a lot for doing this! Will drop more questions if they come up as I go through the other PRs, but this looks great

proto/stride/records/records.proto Show resolved Hide resolved
proto/stride/records/records.proto Show resolved Hide resolved
proto/stride/records/records.proto Show resolved Hide resolved
@sampocs sampocs changed the base branch from main to v23 June 13, 2024 02:05
@sampocs sampocs merged commit 74c51cb into v23 Jun 13, 2024
1 check 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.

4 participants