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: swapper paranoia ledger signing fix #8577

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Conversation

gomesalexandre
Copy link
Contributor

Description

Follows up on #8570 (the accidental, actual fix for #8574) with another paranoia fix for consistency, which, if we used in the first place, would also be an actual fix.

The reason why Ledger signing was borked was because of useTradeNetworkFeesCryptoBaseUnit calling adapter.getAddress() without a pubKey property, meaning we use on-device derivation to get the address.
This would normally be fine when we're able to get an address from the Ledger, we definitely should prefer that vs. using a pubKey we got somewhere from the app state, unfortunately, that's not always possible.
Unfortunately, in this precise case, it was not: making a device call to get the address while Tx signing is in-flight will result in an error being thrown, which will throw (pun intended) swapper into an errored state itself.

That shouldn't be an issue anymore after #8570, but, just for the sake of consistency and paranoia, this PR ensures we don't do on-device derivation here and pass the pubKey in instead, resulting in either it being used directly for address-based accounts, or using unchained to derive addy from xpub using unchained for UTXOs.

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

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

Low

Testing

  • Signing with Ledger is still happy with all chain families: UTXOs, EVMs, Cosmos SDK (THOR or Cosmos), Solana
  • For each of these, ensure that, after seeing the signing prompt on-device, you wait some time (say, 15 seconds) before signing, and the swapper doesn't error in that interim

Engineering

  • ^

Operations

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

Screenshots (if applicable)

https://jam.dev/c/11f7e8a6-685d-4357-b15b-bafa82becbb9

@gomesalexandre gomesalexandre requested a review from a team as a code owner January 14, 2025 22:06
Base automatically changed from feat_stop_refetching_hop_executed to develop January 14, 2025 22:15
Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

EVM

image
Had an issue with recording but I did wait around 30 seconds then signed, worked properly!

Cosmos SDK

image

Waiting for outbound, working fine (after waiting 30+ sec before effectively signing the TX)

Solana

https://jam.dev/c/6fdc32af-4ec8-4c88-92db-1d6472b526a8

UTXOs

Thorchain outbound TX is taking a lot of time, no reasons why it wouldn't work regarding the code, I'll leave this to OPS and/or test it when outbound arrived

Overall looks fine!

@gomesalexandre gomesalexandre enabled auto-merge (squash) January 15, 2025 10:02
@gomesalexandre gomesalexandre merged commit 225d25f into develop Jan 15, 2025
3 checks passed
@gomesalexandre gomesalexandre deleted the fix_ledger_paranoia branch January 15, 2025 10:09
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