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

feat: consolidate thor logic #6700

Merged
merged 57 commits into from
May 1, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Apr 16, 2024

Description

This PR extracts the Tx building logic into a common THOR hook - consuming it in THORChain LP as a first step everywhere.

See #6708 and #6729 for the granular PRs that implement lending and savers and their own testing steps

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)

Risk

High Risk PRs Require 2 approvals

High, touches THORChain LP, lending and savers

What protocols, transaction types or contract interactions might be affected by this PR?

Testing

Regression test the following for LP:

  • sym/asym deposits and withdraws
  • At least one EVM native asset and EVM token for each of the above
  • At least one non-EVM asset for each of the above

Refer to the #6708 and #6729 tickets for lending and savers testing.

Engineering

  • ^

Operations

  • ^

Screenshots (if applicable)

  • Asym token deposit
image image
  • Asym token withdraw (disregard the error, the unsafe minimum has been monkey patched to test this)
image image
  • Asym native EVM asset deposit
image image
  • Asym native EVM asset withdraw
image image
  • Asym RUNE deposit
image image
  • Asym RUNE withdraw
image image
  • Sym EVM native asset deposit
image image
  • Sym EVM native asset withdraw
image image

@gomesalexandre gomesalexandre changed the title [skip ci] wip: consolidate thor logic p1: thorchain LP feat: consolidate thor logic p1: thorchain LP Apr 17, 2024
@gomesalexandre gomesalexandre marked this pull request as ready for review April 17, 2024 12:44
@gomesalexandre gomesalexandre requested a review from a team as a code owner April 17, 2024 12:44
@gomesalexandre
Copy link
Contributor Author

Opening for early review and discussion - while this is already working and wired up for THORChain LP, happy to discuss the API some more here before we get this in, since this is eventually going to be consumed by all THORChain LP domains.

@gomesalexandre gomesalexandre force-pushed the feat_thor_consolidate_tx_build_send_p1 branch 2 times, most recently from 6827641 to ab744f8 Compare April 22, 2024 10:38
@woodenfurniture
Copy link
Member

woodenfurniture commented Apr 30, 2024

Teting notes

LP

🚫 Blank screen on click "add liquidity" button

Screen.Recording.2024-05-01.at.7.36.46.AM.mov

🚫 (May not be a regression) text on confirmation says "Transaction submitted" instead of "Transaction confirmed"

image


🚫 (Possibly related to the above) text after confirming still says "Waiting for confirmation" instead of "Transaction submitted"

Screen.Recording.2024-05-01.at.8.47.03.AM.mov

Add liquidity - non EVM

✅ Native deposit https://viewblock.io/thorchain/tx/390bc58d599f0e4672809e1d7a46fffb6937506f15f42df47405b162614a5aa6

image


✅ Rune deposit https://viewblock.io/thorchain/tx/9E89C1C93A271EB5AE43959C7474B07B339B1D6B544824D8BAB96C26BA0D9E89

image


TODO: Sym deposit


Add liquidity - Native EVM

✅ Asym deposit
https://viewblock.io/thorchain/tx/8b896aa5454a8a3cbbfdca23c2020354f9ee3513a285abeccaa85c347bfb65fc

image


🚫 ~Sym deposit - stuck after rune side completes. ~ link to issue #6787
https://viewblock.io/thorchain/tx/9283E5816B31A73A37D1009B456EFD8A76D869ED446B6790711FDFC53326E739

This issue looks unrelated to this PR because it seems to be an issue where we arent able to prevent conflicting deposit types across tabs. The following message appeared after sym rune deposit - but there was already a sym deposit in flight in a different tab... The seems to warrant some discussion.

image

image


✅ Rune asym deposit https://viewblock.io/thorchain/tx/4B368941F763628989260E328D0FC3C41C297A82882448D86C506CB1308DC577
image


Remove liquidity

✅ Remove liquidity - sym non-EVM https://viewblock.io/thorchain/tx/CECA6B5850A95DD1B9DB94A672A02A77CFC00D04D717A903C02153911F516B22

image


TODO: Remove liquidity - asym non-EVM (RUNE)


TODO: Remove liquidity - asym non-EVM (DOGE)


✅ Remove liquidity - asym EVM (AVAX) https://viewblock.io/thorchain/tx/011417992033214ba9550d4cdad67e2f8f71bf2451107f64aa35731f4774885a

image


✅ Remove liquidity - asym EVM (AVAX) https://viewblock.io/thorchain/tx/28663DE796B9F30B55E52FCC3C479536EE29AC4B16B72D181A4A16FD4A8CF066

image


TODO: Remove liquidity - asym EVM (RUNE)


TODO: add/remove liquidity for non-native EVM sym/asym/rune

Other / Paranoia

✅ ETH -> AVAX bridge
✅ ETH -> RUNE bridge
✅ ETH -> DOGE bridge

@kaladinlight kaladinlight merged commit 1a24da4 into develop May 1, 2024
3 checks passed
@kaladinlight kaladinlight deleted the feat_thor_consolidate_tx_build_send_p1 branch May 1, 2024 16:08
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.

Consolidate Tx building into a common util over all THOR domains
3 participants