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: thorchain lp ledger open app ack #7632

Merged
merged 7 commits into from
Aug 28, 2024
Merged

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Aug 27, 2024

Description

Does precisely what you'd expect it to do based on its box content, minus deposit/withdraw execution bits (implemented in useSendThorTx as part of #7628)

Issue (if applicable)

closes #7580

Risk

High Risk PRs Require 2 approvals

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

Testing

  • THORChain LP approval displays the Ledger Open App ack
  • THORChain LP deposit displays the Ledger Open App ack
  • THORChain LP withdraw displays the Ledger Open App ack

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

  • Approve
Screenshot 2024-08-27 at 17 52 07
  • Add Liquidity
Screenshot 2024-08-27 at 17 57 13
  • Remove Liquidity
Screenshot 2024-08-27 at 18 06 02

@gomesalexandre gomesalexandre requested a review from a team as a code owner August 27, 2024 16:11
@gomesalexandre gomesalexandre marked this pull request as draft August 27, 2024 16:13
@gomesalexandre gomesalexandre marked this pull request as ready for review August 27, 2024 16:13
@kaladinlight
Copy link
Contributor

Looks like we are missing this check in Sweep which is a possible flow in Thorchain LP. Also noticing we simply haven't added this check around standard sends useFormSend as a side note

@gomesalexandre
Copy link
Contributor Author

Looks like we are missing this check in Sweep which is a possible flow in Thorchain LP. Also noticing we simply haven't added this check around standard sends useFormSend as a side note

Actually very valid side note, added it in handleSend will both handle sweeps and regular sends - 39154e5:

  • Add Liquidity Sweep
image
  • THORChain Savers Deposit Sweep
image

Note sends from the send modal are currently borked in this diff - to be tackled in a follow-up as well as we can't open a modal within a modal and we'll need to update the Modal provider to accomodate that.

Note the double checks in useSendThorTx's executeTransaction, which are expected for the time being (the first because of the current accountNumber shenanigans, fixed in #7635 above, the second being the actual one we want).
Going to follow-up with a holistic change at adapters-level so adapter methods that rely on on-device derivation take checkLedgerAppOpenIfLedgerConnected in instead, so this logic is better encapsulated and most importantly, so we are required to pass it.

@kaladinlight
Copy link
Contributor

Note the double checks in useSendThorTx's executeTransaction, which are expected for the time being (the first because of the current accountNumber shenanigans, fixed in #7635 above, the second being the actual one we want). Going to follow-up with a holistic change at adapters-level so adapter methods that rely on on-device derivation take checkLedgerAppOpenIfLedgerConnected in instead, so this logic is better encapsulated and most importantly, so we are required to pass it.

On the same page with the holistic update. Current setup not super manageable and prone to regressions or forgetting on new implementations. Handling at adapter level makes a lot of sense. Wondering if we could leverage event emitters and have it completely encapsulated in chain adapters to reduce the need to propagate the function pass through everywhere. Then we can just listen for the ledger app open event and show the modal accordingly.

Copy link
Contributor

@kaladinlight kaladinlight left a comment

Choose a reason for hiding this comment

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

approving - noting send modal will be tackled as a follow up. also, holy log spew for this hook, going to add a quick follow up to silence that as well

@gomesalexandre gomesalexandre enabled auto-merge (squash) August 28, 2024 22:20
@gomesalexandre gomesalexandre merged commit 1e17510 into develop Aug 28, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the feat_ledger_ack_lp branch August 28, 2024 22:26
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.

Ledger open app gate in THORChain LP
2 participants