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

Handle precision error case in tokenize shares and change shareToken to map 1:1 with shares #19

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Jul 14, 2023

High level changelog

  1. Handled precision error case that would cause tokenizations to fail
  2. Changed the LSM share tokens to map 1:1 with shares instead of tokens

I would recommend viewing this PR commit by commit to differentiate these two changes

Context on shares and tokens

In the SDK, delegations are denominated using shares - and are converted to tokens when necessary using the validator's exchange rate. This way, when a validator is slashed, the number of shares remains the same, but the number of tokens (and thus the exchange rate) decreases.

Example:

  1. Validator X has 1M ATOM (tokens) and 1M shares (exchange rate of 1.0)
  2. Alice delegates 1000 ATOM to Validator X
    → This creates a delegation record with 1000 shares
  3. Validator X is slashed by 5%
    → Now Validator X has only 950k tokens, but still 1M shares (exchange rate of 0.95)
    → Alice's delegation still has 1000 shares, but those shares are now worth 950 tokens
  4. Bob delegates 1000 ATOM to Validator X
    → Bob receives 1052.6 shares (1000 ATOM / 0.95 exchange rate = 1052.6 shares)
    → Those shares are worth 1000 ATOM (1052.6 shares * 0.95 exchange rate = 1000 ATOM)

Change #​1 - Handle precision error during tokenization

Context

Due to the decimal to integer conversion that happens between shares and tokens, there is the potential for precision error to cause the loss of a token during unbonding. This situation has been present in the SDK and was not a consequence of LSM. Often on mainnet cosmoshub, if a validator has had more than 1 slash and has a long decimal exchange rate, undelegating X will return only X-1 tokens.

How this relates to LSM: During the TokenizeShares transaction, the tokenizer's delegation record is removed and replaced with a new delegation record that has a TokenizeShareModuleAccount as the owner. Similarly, in RedeemTokensForShares, the TokenizeShareModuleAccount's delegation record is replaced with a new one owned by the tokenizer. Under the hood, in both of these functions, the Unbond function is called to remove the original record, and then Delegate is called to create the new one.

The Issue: TokenizeShares attempts to call Unbond and Delegate with the same amount, but if the Unbond step returned 1 less token due to a precision error, the Delegate step would fail due to insufficient funds. The impact is that it would cause tokenization's to fail anytime there was precision error.

Changelog

  • Removed delegation shares check - this check is covered below, but would automatically error if the precision error occurred
  • Changed Delegate call in Tokenization to use the amount returned from the Unbond call, instead of the amount specified as a message parameter
  • Moved mint/send of LSM Share token to after Unbond call, where the new amount isn't known
  • Renamed msg.Amount variable for clarify

Change #​2 - Changed LSM share tokens to map 1:1 with shares

Context

Currently LSM share tokens are returned 1:1 based on the number of delegated tokens that are tokenized (e.g. tokenizing 1 ATOM, returns 1 LSM share token). However, it makes more sense to return the share tokens such that they map 1:1 with delegation's shares, for the following reasons:

  • Shares is used internally to represent delegations so this better aligns with the underlying implementation
  • The value of the LSM share token is unclear when denominated in tokens because a slash could have occurred after the tokenization step. This makes the conversion complex as there is a timing element. If denominated in shares, the value can easily be determined using the validator's exchange rate

Changelog

Since shares is used under the hood, the only necessary changes here were on the minting/burning of LSM share tokens in TokenizeShares and RedeemTokensForShares - no accounting or record keeping changes were required.

  • Changed TokenizeShares to return share tokens 1:1 with shares
  • Simplified RedeemTokensForShares by removing conversion of shareToken to shares (as it is now unnecessary)
  • Added case to handle precision error - if the whole tokenize share record is being redeemed, rounds up to make sure it covers the full delegation (same way it's done with unbondings)
  • Added unit tests to cover slashes and rounding/conversion imprecision
  • Updated ADR

@xlab
Copy link

xlab commented Jul 18, 2023

Can confirm that this PR actually fixes a broken invariant during sims:

cd simapp && go test . -run TestAppImportExport -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=124 -Period=5 -ExportParamsPath -v -timeout 24h
--- FAIL: TestAppImportExport (8.69s)
panic: invariant broken: staking: bonded and not bonded module account coins invariant
	Pool's bonded tokens: 3260347594636stake
	sum of bonded tokens: 3260347594636
not bonded token invariance:
	Pool's not bonded tokens: 12032682897824stake
	sum of not bonded tokens: 12032682897825
module accounts total (bonded + not bonded):
	Module Accounts' tokens: 15293030492460stake
	sum tokens:              15293030492461


	CRITICAL please submit the following transaction:
		 tx crisis invariant-broken staking module-accounts [recovered]

Well done 👍

Copy link
Collaborator

@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.

Great work on this. Clear docs, clean code and thorough tests.

@sampocs sampocs merged commit a432395 into v0.45.16-ics-lsm Jul 20, 2023
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