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

fix: remove addSlippageToMemo in favor of addLimitToMemo #7138

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

NeOMakinG
Copy link
Collaborator

Description

  • I removed addSlippageToMemo because the only thing it does is taking the slippage out of the limit and putting it back, the role of putting it in the memo should be limited to addLimitToMemo and this limit should be precalculated
  • To calculate the limit I used a util getLimitWithManualSlippage then I finally used addLimitToMemo

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

partially #6777

Risk

High as it might add wrong behaviors to minAmountOut of THORChain memos

Testing

Memos and particularly minAmounOut part should be the same as before:
Tests those swaps and compare the minAmountOut values in the memos by adding console logs in getL1quote in every handlers:

  • BTC => ETH
  • BTC => BCH
  • ETH => RUNE
  • ETH => COSMOS
  • SHIBA => BTC
  • DOGE => ETH

Engineering

n/a

Operations

n/a

Screenshots (if applicable)

Rune => BTC

image Amount before: 34217, after slippage: 33874, seems good as it represent 1%

ETH => COSMOS

image

Amount before: 464163000, after slippage: 459521370, seems good as it represent 1%

SHIB => BTC

image

Amount before: 45308, after slippage: 44854, seems good as it represent 1%

@NeOMakinG NeOMakinG requested a review from a team as a code owner June 14, 2024 15:32
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Spot-on! Much cleaner that way and implementation is sane (code moved around).

Confirmed at runtime with 10x dev console.logs limit is still happy i.e 498441*90/100 = 448596.9, and limit is indeed 448596

image

@NeOMakinG NeOMakinG enabled auto-merge (squash) June 17, 2024 08:04
@NeOMakinG NeOMakinG merged commit 5918b98 into develop Jun 17, 2024
3 checks passed
@NeOMakinG NeOMakinG deleted the refactor_memo_parsing branch June 17, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants