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: implement manual receive address to limit orders #8065

Merged
merged 18 commits into from
Nov 8, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Nov 1, 2024

Description

Implements manual receive address entry for limit orders.

Includes a major refactor of manual receive address for trade input to make this feasible:

  • Combines ReceiveAddress with ManualAddressEntry into SharedReceiveAddress to greatly reduce code and logic duplication
  • Decouples everything trade specific from receive address logic for re-use with limit orders
  • Adds some hooks to dedupe logic and make is simpler to access

Includes minor fixes:

  • If a user disconnects a non-ledger wallet from a chain, we don't currently tell them that connecting the chain is an option and force them to input a manual receive address. Fixed in this PR.

There are 2 main ways to manually set a recipient address:

  1. "custom recipient address"
  • a user decides they want their trade to land in a different wallet
  1. "manual recipient address entry"
  • the connected wallet doesn't CURRENTLY have an account supporting the buy asset chain
  • the connected wallet may have "runtime support" meaning it's possible to do some actions (installing snaps, doing something on the device or adding an account via account management) to make the wallet support the chain
  • in this case a user is required to manually enter a recipient address, or enable snaps on metamask, or enable the chain via account management

When a manual recipient address is entered:

  • the address entered must pass validation
  • it must be possible to "cancel" and return to the original state

Issue (if applicable)

Progresses limit orders.
Incidentally, closes #8102

Risk

High Risk PRs Require 2 approvals

⚠️⚠️⚠️⚠️💀💀💀💀 About as high risk as it gets 💀💀💀💀⚠️⚠️⚠️⚠️

This PR is high risk because it modifies code that directly impacts where user funds land. If there is a bug in this code, it could result in lost funds.

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

All protocols for trades.

Testing

Note this issue in prod before testing:
#8068

Should be functionally identical to production in terms of:

  • funds land in the specified account - test this THOROUGHLY, both with and without manual receive address!
  • address entered is validated (invalid, valid, etc)
  • setting, cancelling, editing recipient addresses

As this PR combines some of the components, it's expected that the UI will be slightly different to prod, and more consistent between the 2 ways of entering the recipient address (see above for what those 2 ways are).

Things to test:

  • metamask without snap, being prompted to install it
  • wallet without buy asset chain connected (i.e disconnected via account management), and being prompted to connect it (via account management)

Engineering

Operations

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

Screenshots (if applicable)

User prompted to connect account via account management
https://jam.dev/c/e1c63699-7eec-47d5-bbca-517875b2f710

User prompted to install snap
https://jam.dev/c/14d1f44b-31a2-46c5-9218-4b788cc67553

Setting a custom recipient address
https://jam.dev/c/d4f82eb3-78df-4d7f-baaa-b760609b8a86

RUNE -> BTC with custom recipient address
https://jam.dev/c/985c3eba-c46b-4e17-aadd-2b4fe797f29f
https://viewblock.io/thorchain/tx/CA735E3A5ACE40AC15015F20B62C2E94E845AC7D329CC8B1B05DCBCE7355B502

FOX.ETH -> AVAX with custom recipient address
https://jam.dev/c/30f485ac-e2d5-4ccf-bd56-822e145427f2

@woodenfurniture woodenfurniture marked this pull request as ready for review November 3, 2024 23:57
@woodenfurniture woodenfurniture requested a review from a team as a code owner November 3, 2024 23:57
@0xean 0xean requested review from 0xApotheosis, kaladinlight and gomesalexandre and removed request for kaladinlight November 4, 2024 22:05
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.

Eggcellent. Conceptually LGTM minus a few naming nitpicks - only a small oneliner blocker re: MM desktop wallet checks.

Runtime pass to follow 🙏🏽

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.

Runtime pass after applying the checkIsMetaMaskDesktop checks locally

tl;dr a few issue re: flashes but functionally looks good overall 🙏🏽

https://jam.dev/c/999cf6ca-c35b-4c24-8768-f382179205f9

Works fine for the buy side, and with correct connect chain CTA now!
Sell side I'm not sure whether or not should be considered a regression or an improvement: develop swaps chains + toggles custom receive address display, while this diff doesn't capture unsupported sell side, but doesn't do chains swap.
IMO much saner now, and not worth capturing loss of sell side support, especially with the fact that we're going with rates and a connect wallet CTA very, very soon!

  • native fresh load without cache is still happy and not showing weird jumpy manual receive addy 🚫

Noticing
- some smolish flash (here around 10 seconds)
- Recipient Address component is showing up empty during load, while it is not displayed in develop

https://jam.dev/c/245dd918-ed8c-4c89-9863-d698cf43b2f7

Confirmed this is a regression against develop:

https://jam.dev/c/fe80e86a-653a-4eb2-8e0a-34a8f89e206f

  • MM snaps fresh load without cache is still happy and not showing weird jumpy manual receive addy 🚫

Noticing a bad boi flash:

https://jam.dev/c/820947d0-dead-4ef4-bf51-58c54aeb0c65

Which isn't present in develop:

https://jam.dev/c/f536d759-a5c4-4889-b9bd-feae12326921

https://jam.dev/c/6908b7f9-2255-458f-9e3b-5d1666f373b4

  • funds land on the correct wallet - native with custom receive addy ✅

https://jam.dev/c/8cfb1a52-bc5b-436b-bd21-dd147a27c559

  • Manual address validation is still happy / Playing around with manual receive addy (toggling, removing, editing) is still happy ✅
    https://jam.dev/c/ca9193df-d834-41a4-a103-0f314f5b9dcf

  • Smart contract address checks are still happy (i.e inputting a manual receive addy which is a sc on the wrong chain should fail) ✅

Disregard this one, confirmed it's possible to input a custom sc receive addy both in this diff and develop, though since we're talking receive side and custom, this may or may not be in the realm of "let users input what they want"? Would be a relatively simple addition for safety if we want to however!

  • MM no snaps still prompt for snap install or manual receive addy when trying to use an unsupported chain as buy asset ✅

https://jam.dev/c/a93ce747-8c07-4bb7-b84e-beb11876d48a

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

First pass code-review, runtime testing to follow.

Looking good ser.

queryFn: isInitializing
? skipToken
: async () => {
// Already covered in shouldSkip, but TypeScript lyfe mang.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still valid? There is no shouldSkip reference

Copy link
Contributor

@gomesalexandre gomesalexandre Nov 6, 2024

Choose a reason for hiding this comment

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

Also if we want type safety here as well as explicitly skipping vs. firing a query and early returning from it we can explicitly re-add the checks from isInitializing (i.e the checks belowin addition toisInitializing` itself i.e

Suggested change
// Already covered in shouldSkip, but TypeScript lyfe mang.
`isInitializing && (!buyAsset || !wallet || !buyAccountId || !buyAccountMetadata)`

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.

Retested with checkIsMetaMaskDesktop diff (https://github.com/shapeshift/web/pull/8065/files#r1828534966) applied locally - confirmed flashes are gone.
Other issues still present, and noticed something odd re: disconnecting previously connected buy account, which may or may not be intended.
Most importantly, noticed the snap is now broken and doesn't fetch non-EVM accounts.

  • Non-EVM accounts aren't fetched anymore with MM snaps 🚫

https://jam.dev/c/a6224e50-f358-4e00-ba5c-6776843f4aab

  • confirmed fixed: switching assets consistently triggers a flash of manual receive address state

https://jam.dev/c/e86200f5-fb5f-4f15-9b46-1cadfa7ccbce

  • Accounts load flash is gone ✅

https://jam.dev/c/4b9e027e-37af-4fc0-838b-e539816cd43d

  • Recipient Address component is still showing up empty during accounts load 🚫

https://jam.dev/c/4b9e027e-37af-4fc0-838b-e539816cd43d

  • wallet with disconnected chain prompts for connection when trying to use an unsupported chain as buy asset :question_mark:

Not sure whether that's a feature or a bug, but the previously connected account is still shown as a recipient, manual-looking address after disconnect:

https://jam.dev/c/1c5d21c1-bbb2-4ecd-a4ac-99f5c5f7ec15

  • MM snaps fresh load without cache is still happy and not showing weird jumpy manual receive addy ✅

https://jam.dev/c/20963e1b-5230-4f2f-ac07-f02614ab8422

  • switching from native to MM is still happy without custom receive addy flash ✅

https://jam.dev/c/5dad9b6d-bcec-485b-84c5-aa6c095dcd64

  • Manual address validation is still happy / Playing around with manual receive addy (toggling, removing, editing) is still happy ✅

Possibly unrelated to this PR but noticing wrong checksum is failing verification, which it shouldn't since EVM addies may well be copied not checksummed

https://jam.dev/c/54963a0d-a7cb-43b8-a3e9-98b2483649ac

@woodenfurniture
Copy link
Member Author

Non-EVM accounts aren't fetched anymore with MM snaps 🚫

Appears to be happy again after actioning all review feedback 🎉

https://jam.dev/c/e96ec9a3-ad96-49ca-b569-f25a6d03586c

Recipient Address component is still showing up empty during accounts load 🚫

Fixed in b5d3af7

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.

Third time's a charm! Get in ser

https://gist.github.com/gomesalexandre/9afcd2fdf1bd0a8f0b9bf57a2819e823

Snap bug captured in #8096

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

✅ funds land in the specified account - test this THOROUGHLY, both with and without manual receive address!

Without manual receive address

Screenshot 2024-11-07 at 13 50 25
  • With manual receive address
Screenshot 2024-11-07 at 13 54 02

✅ address entered is validated (invalid, valid, etc)

✅ setting, cancelling, editing recipient addresses

❌ metamask without snap, being prompted to install it

Not prompted, but I suspect that's due to the issue below 👇

❌ wallet without buy asset chain connected (i.e disconnected via account management), and being prompted to connect it (via account management)

I don't seem able to get a quote into a buy asset without a wallet receive address after disconnecting a chain (Avalanche in the screenshot below):

Screenshot 2024-11-07 at 13 59 44

@woodenfurniture
Copy link
Member Author

❌ metamask without snap, being prompted to install it

Not prompted, but I suspect that's due to the issue below 👇

❌ wallet without buy asset chain connected (i.e disconnected via account management), and being prompted to connect it (via account management)

@0xApotheosis very VERY well spotted. This was caused by an existing issue of state corruption in redux, masked by additional protections implemented by @gomesalexandre 🙏🙏🙏🙏 which prevents this from being a production issue.

The issue in redux was that the account IDs are not reset when the asset changes, meaning it's possible for redux to store the incorrect account ID for a given asset.

The fix was to:

  • reset the account ID in redux when a selected asset changes (via selection or switching)
  • reset any manual receive address when a selected asset changes (via selection or switching)
  • move the safety check back into the original code location

1a874c1

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Something is still off here. When attempting to trade using a manual receive address:

Error: missing receiveAccountNumber

Screenshot 2024-11-07 at 16 04 45

Replicated with both MetaMark (without Snaps) and Native with an account disabled.

@woodenfurniture
Copy link
Member Author

woodenfurniture commented Nov 7, 2024

Something is still off here. When attempting to trade using a manual receive address:

Error: missing receiveAccountNumber

Replicated with both MetaMark (without Snaps) and Native with an account disabled.

Looks like a production issue and unrelated to this PR:
https://jam.dev/c/6d48a2a4-f13b-4cf8-b649-90fc5691b8db

I'll go ahead and log a ticket but lets get this PR in before tackling
#8102

@0xApotheosis
Copy link
Contributor

Sorry @woodenfurniture, I'm not convinced this isn't a regression - let's pile on it.

Left is this branch, right production. The issue seems unique to this branch.

Screenshot 2024-11-08 at 09 47 33

@woodenfurniture
Copy link
Member Author

Sorry @woodenfurniture, I'm not convinced this isn't a regression - let's pile on it.

Left is this branch, right production. The issue seems unique to this branch.

Screenshot 2024-11-08 at 09 47 33

Can confirm, it's easier to repro in in this branch than prod. Prod requires changes to accounts config to repro, whereas this is consistently broken.

Fixed in 0e299a9 🎉

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

We got there, this is looking good. Banger PR ser, well done. Let's get 'er in.

Confirming final issue was resolved in 0e299a9.

@0xApotheosis 0xApotheosis merged commit ccdcd4f into develop Nov 8, 2024
3 checks passed
@0xApotheosis 0xApotheosis deleted the limit-orders-receive-address branch November 8, 2024 01:45
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.

Unable to trade to manual receive address when account disabled
3 participants