feat: holistic custom recipient address part 2 #6072

gomesalexandre · 2024-01-24T17:45:41Z

Description

Adds custom recipient address support holistically, regardless of whether or not the currently connected wallet does/not support the receive chain

TODO:
- [x] add form context instead of handrolled logic and move me to a dedicated component
- [x] dispatch on save only, but still validate on form value change
- [x] ensure smart contract checks are honored
- [x] custom/recipient states
- [ ] styling
- [x] If the user hovers over the tag they will see a tooltip that says, "This is your custom recipient address"
- [ ] We should absolutely not do this. This is by design, and we *need* to refetch quotes ~~loosen receive address reactivity so that we don't get rugged infinitely refreshing on valid recipient~~

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)

relates to Block smart contract receive addresses for SwapOut longtails #5805 - this technically tackles it fully, however, we have no way to test this yet since long-tails to EVM tokens aren't live yet. To be tested as part of that issue when this is live, moved it to blocked for now.
relates to Custom recipient / custom to address on swaps #5883, part 2 out of 3, enabling the feature will be the last

Risk

High Risk PRs Require 2 approvals

High. While this is theoretically still under flag and there may be another PR to improve things visually, and another to turn the flag on which will give us another chance to test this, eng. and ops should assume this is going directly to production with no flag.

Testing

⚠️ Test all the below both with a wallet supporting the chain you're swapping to, and one which does not
⚠️ Test this both this feature with the flag on, and regression test it with the flag off as enabling it is going to go in as a separate follow-up PR

Custom recipient address is honored fow all swaps. EVM swappers, THOR, streaming/long-tail, you name it.
Validation is working - entering an address for a wrong chain or something that's not a valid address doesn't let you continue
Ensure going back and forth from custom to default wallet address brings no regressions
While this was not introduced by this PR, this will now expose this in a much bigger area: we're leveraging the address validation we have for sends, meaning it is possible to input an ENS name. Ensure this doesn't result in lost user funds, in particular with THORCHain.

^Note WRT the above: this shouldn't be an issue as the custom recipient will be used to get a quote, so instead of producing bugs, this will simply limit the amount of available swappers when using an ENS address (because the missing swappers do not support it, hence do not yield a quote)

Ensure THORChain custom recipients do not result in loss funds. While this should not be an issue for regular swaps (only long-tail swaps to EVM chains -which we do not support yet - with a smart contract address as destination are affected by the smart contract restriction), it may.
Ensure account selection does not rug custom recipient

Engineering

Operations

Screenshots (if applicable)

Invalid address

Default wallet address

Custom recipient valid address input

Set custom recipient

<img width="787" alt="Screenshot 2024-01-24 at 23 34 27" src="https://github.com/shapeshift/web/assets/17035424/3db140d9-8153-4ef7-83ff-c897d22c0ddd"

Setting an ENS as custom recipient works and restricts the available swappers on swappers supporting it

Screen.Recording.2024-01-24.at.23.41.02.mov

1inch account 0 sell address, ETH account 1 as custom recipient

Li.Fi account 0 sell address, ETH account 1 as custom recipient

Random DOGE address with no balance as custom recipient

Wallet not supporting the receive asset

Notice how the new custom recipient kicks in instead of the old receive address component as soon as we get an active quote, meaning we were able to using a custom recipient

Screen.Recording.2024-01-25.at.00.07.50.mov

Flag off

Screen.Recording.2024-01-25.at.00.07.50.mov

gomesalexandre · 2024-01-24T17:45:54Z

Current dependencies on/for this PR:

develop
- PR fix: synchronize buyAssetAccountId to sellAssetAccountNumber on swapper mount #6124
  - PR feat: holistic custom recipient address part 2 #6072 👈

This stack of pull requests is managed by Graphite.

gomesalexandre · 2024-01-24T23:19:32Z

src/components/MultiHopTrade/components/TradeInput/components/ManualAddressEntry.tsx

@@ -64,7 +64,7 @@ export const ManualAddressEntry: FC = memo((): JSX.Element | null => {
  // If we have a valid manual receive address, set it in the form
  useEffect(() => {
    manualReceiveAddress && setFormValue(SendFormFields.Input, manualReceiveAddress)
-  }, [dispatch, manualReceiveAddress, setFormValue])
+  }, [manualReceiveAddress, setFormValue])


This was not needed but somehow not caught by the linter

src/components/MultiHopTrade/components/TradeInput/components/ManualAddressEntry.tsx

src/components/MultiHopTrade/components/TradeConfirm/ReceiveSummary.tsx

src/components/MultiHopTrade/components/TradeInput/components/ManualAddressEntry.tsx

gomesalexandre · 2024-01-24T23:25:05Z

src/components/MultiHopTrade/components/TradeInput/components/RecipientAddress.tsx

+  const isYatSupported = isYatFeatureEnabled && isYatSupportedByReceiveChain
+  const {
+    formState: { isValidating, isValid },
+    // trigger: formTrigger, // TODO(gomes): do we need this?


Question for @0xApotheosis since this bit was taken from the previous custom receive address component

I'm going to go with "No" - I don't remember this guy's story, but I suspect it can go.

woodenfurniture · 2024-01-25T06:57:03Z

src/components/MultiHopTrade/components/TradeInput/TradeInput.tsx

+      tradeQuoteStep?.source === THORCHAIN_LONGTAIL_STREAMING_SWAP_SOURCE &&
+      _isSmartContractReceiveAddress !== false
+    )
+      return true


might also need to update
src/state/apis/swapper/helpers/validateTradeQuote.ts -> disableSmartContractSwap

Nice catch, will tackle 🙏🏽 also noticed we've apparently broken the smart contract checks in https://github.com/shapeshift/web/pull/5971/files#diff-b6f4d6672824399e71740531e3b09efe3ad3fabeb3ad6c4ce0763969f9d39ab7R210, will author a hotfix in the meantime (see receiveAddress vs. senderAddress)

Restacked on top of fix: regression of smart contract sell address checks on validateTradeQuote #6081

Added receive side checks and additional comments in 639cd28

made this PR relate to Block smart contract receive addresses for SwapOut longtails #5805 instead of closing it while at it, since we cannot yet test this until L1 -> EVM long-tails are implemented in Add L1 to Ethereum long-tail swaps #5892

woodenfurniture · 2024-01-25T06:58:03Z

src/components/MultiHopTrade/components/TradeConfirm/ReceiveSummary.tsx

@@ -351,77 +290,7 @@ export const ReceiveSummary: FC<ReceiveSummaryProps> = memo(
                  </Row>
                </>
              )}
-
-            {/* TODO(gomes): This should probably be made its own component and <ManualAddressEntry /> removed */}
-            {/* TODO(gomes): we can safely remove this condition when this feature goes live */}


src/components/MultiHopTrade/components/TradeInput/components/RecipientAddress.tsx

woodenfurniture

Code wise looks good apart from 1 blocking comment. Will need to test thoroughly when back online

gomesalexandre · 2024-02-02T08:58:15Z

Cross-account issue captured in #6121

…e_address_p2

gomesalexandre · 2024-02-02T11:25:17Z

More functional testing:

I was consistently able to get into a sad state by:

Setting a custom address

Removing it

Typing a custom address, deleting it, and pressing "Preview Trade" (some combination of this, I'm not sure exactly - see video)

We should disable the check button (and possibly "Preview Trade") when no input is provided, as it does nothing in this case.

@0xApotheosis while this is still in draft for the prod issue noted above, this has been fixed in 74b9250:

Screen.Recording.2024-02-02.at.12.18.33.mov

Also actioned the streaming swap longtail source comment in 7228126

This is ready to be retested, but can't be merged until the prod issue is tackled.

swapper mount

…ive_address_p2

0xApotheosis

❌ THORSwap trades now seem broken (see comment inline)

✅ Check mark is now disabled when no valid address entered.

❌ "Preview Trade" button is not disabled when no address is present (e.g. when entering a custom receive address, before pressing the green check to save it). Pressing "Preview Trade" at this time throws an exception:

0xApotheosis · 2024-02-04T22:28:07Z

src/state/apis/swapper/helpers/validateTradeQuote.ts

+    if (
+      [firstHop.source, secondHop.source].some(source =>
+        [THORCHAIN_LONGTAIL_SWAP_SOURCE, THORCHAIN_LONGTAIL_STREAMING_SWAP_SOURCE].includes(source),
+      ) &&
+      _isSmartContractReceiveAddress !== false
+    )


I think we've now broken quotes from THORChain. The network response returns correctly, but we get the below (with and without a custom receive address):

That one was actually quite the rabbit-hole!

tl;dr of what's up here is we had absolutely no type-safety on steps.length which could (according to types) be of any n-length, and in the specific case of THOR, secondHop was defined here.
Fixed this by bringing in the context of single and multi-hop trade quotes, the former consisting of 1 step, the latter consisting of 2 (for now, as we only have a max. of two hops here).
This now brings type errors which should've been present before e.g in here - 1bbd41a

Also fixed the something went very wrong error in 60b5672 - we got too safe here as there are always going to be at least two renders without a receive address as things are loading

After bringing in safety (and removing the wrong one for receiveAddress checks), THOR is now happy:

with regular swaps and default receive address

with streaming swaps and vitalik.eth injected wallet

packages/swapper/src/types.ts

0xApotheosis

🤔 Probably not a blocker, as we no longer crash in this state, but we still don't ensure the "Preview Trade" button is disabled whilst manually editing the address

✅ THORChain trades are great again

✅ Manual receive address works as expected when manually input

✅ Correct default (synced) address used as default

0xApotheosis

Sorry gomes, that last commit made it much worse - I was able to get into a super stuck state where the "Preview Trade" button was never enabled.
Maybe we just revert and merge as-is.

This reverts commit c0982e5.

0xApotheosis

Re-stamp after revert.

gomesalexandre added the high risk label Jan 24, 2024

gomesalexandre changed the title ~~feat: manual receive address part 2~~ feat: holistic custom recipient address part 2 Jan 24, 2024

gomesalexandre commented Jan 24, 2024

View reviewed changes

src/components/MultiHopTrade/components/TradeInput/components/ManualAddressEntry.tsx Outdated Show resolved Hide resolved

gomesalexandre force-pushed the feat_manual_receive_address_p2 branch from 9e440d6 to 23bf07f Compare January 24, 2024 23:20

gomesalexandre commented Jan 24, 2024

View reviewed changes

src/components/MultiHopTrade/components/TradeConfirm/ReceiveSummary.tsx Outdated Show resolved Hide resolved

gomesalexandre commented Jan 24, 2024

View reviewed changes

src/components/MultiHopTrade/components/TradeInput/components/ManualAddressEntry.tsx Outdated Show resolved Hide resolved

gomesalexandre commented Jan 24, 2024

View reviewed changes

gomesalexandre marked this pull request as ready for review January 24, 2024 23:25

gomesalexandre requested a review from a team as a code owner January 24, 2024 23:25

woodenfurniture reviewed Jan 25, 2024

View reviewed changes

src/components/MultiHopTrade/components/TradeInput/components/RecipientAddress.tsx Outdated Show resolved Hide resolved

woodenfurniture reviewed Jan 25, 2024

View reviewed changes

src/components/MultiHopTrade/components/TradeInput/components/RecipientAddress.tsx Outdated Show resolved Hide resolved

woodenfurniture reviewed Jan 25, 2024

View reviewed changes

This was referenced Jan 25, 2024

Block smart contract receive addresses for SwapOut longtails #5805

Closed

Add L1 to Ethereum long-tail swaps #5892

Closed

gomesalexandre force-pushed the feat_manual_receive_address_p2 branch from 2a8f818 to 4a3e2e5 Compare January 25, 2024 10:30

gomesalexandre changed the base branch from develop to fix_smart_contract_validateQuote_regression January 25, 2024 10:30

gomesalexandre marked this pull request as draft January 25, 2024 10:31

gomesalexandre mentioned this pull request Jan 25, 2024

fix: regression of smart contract sell address checks on validateTradeQuote #6081

Merged

3 tasks

gomesalexandre force-pushed the feat_manual_receive_address_p2 branch from 4a3e2e5 to 639cd28 Compare January 25, 2024 10:37

gomesalexandre marked this pull request as ready for review January 25, 2024 10:40

gomesalexandre added 6 commits January 25, 2024 13:31

fix: smart contract sell address checks on validateTradeQuote regression

5bc463c

feat: manual receive address part 2

79abc91

feat: expose manual and wallet receive address

0313b0b

feat: submit on save click

bf8b4f2

feat: handle custom/receive states

4a390ae

feat: disable smart contract receive address for THOR long-tail

c51a92b

gomesalexandre added 3 commits February 2, 2024 12:20

fix: synchronize valid checks

74b9250

feat: safety check for streaming swap trade source

7228126

Merge remote-tracking branch 'origin/develop' into feat_manual_receiv…

97961e4

…e_address_p2

gomesalexandre added 4 commits February 3, 2024 23:16

fix: synchronize buyAssetAccountId to sellAssetAccountNumber on

0d19af9

swapper mount

fix: ci

7894389

chore: update src/components/MultiHopTrade/hooks/useAccountIds.tsx

75bd1ed

Merge branch 'fix_synchronize_accountids_mount' into feat_manual_rece…

e35380b

…ive_address_p2

gomesalexandre marked this pull request as ready for review February 3, 2024 22:48

Merge branch 'fix_synchronize_accountids_mount' into feat_manual_rece…

5fe5597

…ive_address_p2

gomesalexandre changed the base branch from develop to fix_synchronize_accountids_mount February 3, 2024 22:49

gomesalexandre mentioned this pull request Feb 3, 2024

fix: synchronize buyAssetAccountId to sellAssetAccountNumber on swapper mount #6124

Merged

3 tasks

0xApotheosis requested changes Feb 4, 2024

View reviewed changes

gomesalexandre added 2 commits February 5, 2024 13:07

feat: type safety for MultiHopTradeQuote and SingleHopTradeQuote

1bbd41a

fix: too much safety

60b5672

gomesalexandre commented Feb 5, 2024

View reviewed changes

packages/swapper/src/types.ts Show resolved Hide resolved

0xApotheosis approved these changes Feb 5, 2024

View reviewed changes

feat: disable while editing

c0982e5

0xApotheosis requested changes Feb 5, 2024

View reviewed changes

feat: revert "feat: disable while editing"

c044e8e

This reverts commit c0982e5.

0xApotheosis approved these changes Feb 5, 2024

View reviewed changes

Base automatically changed from fix_synchronize_accountids_mount to develop February 5, 2024 23:13

0xApotheosis and others added 2 commits February 6, 2024 10:22

Merge branch 'develop' into feat_manual_receive_address_p2

403d6e1

fix: merge issue

b99e29e

0xApotheosis merged commit ef60ea7 into develop Feb 5, 2024
4 checks passed

0xApotheosis deleted the feat_manual_receive_address_p2 branch February 5, 2024 23:32

0xApotheosis mentioned this pull request Feb 5, 2024

chore: release v1.532.0 #6129

Merged

This was referenced Feb 6, 2024

feat: disable trade preview while custom recipient is editing #6132

Merged

feat: enable custom recipient address #6133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: holistic custom recipient address part 2 #6072

feat: holistic custom recipient address part 2 #6072

gomesalexandre commented Jan 24, 2024 •

edited

Loading

gomesalexandre commented Jan 24, 2024 •

edited

Loading

gomesalexandre Jan 24, 2024

gomesalexandre Jan 24, 2024

0xApotheosis Feb 2, 2024 •

edited

Loading

woodenfurniture Jan 25, 2024

gomesalexandre Jan 25, 2024 •

edited

Loading

gomesalexandre Jan 25, 2024

woodenfurniture Jan 25, 2024

woodenfurniture left a comment

gomesalexandre commented Feb 2, 2024

gomesalexandre commented Feb 2, 2024

0xApotheosis left a comment •

edited

Loading

0xApotheosis Feb 4, 2024

gomesalexandre Feb 5, 2024 •

edited

Loading

0xApotheosis left a comment

0xApotheosis left a comment

0xApotheosis left a comment

feat: holistic custom recipient address part 2 #6072

feat: holistic custom recipient address part 2 #6072

Conversation

gomesalexandre commented Jan 24, 2024 • edited Loading

Description

Pull Request Type

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

Invalid address

Default wallet address

Custom recipient valid address input

Set custom recipient

Setting an ENS as custom recipient works and restricts the available swappers on swappers supporting it

1inch account 0 sell address, ETH account 1 as custom recipient

Li.Fi account 0 sell address, ETH account 1 as custom recipient

Random DOGE address with no balance as custom recipient

Wallet not supporting the receive asset

Flag off

gomesalexandre commented Jan 24, 2024 • edited Loading

gomesalexandre Jan 24, 2024

Choose a reason for hiding this comment

gomesalexandre Jan 24, 2024

Choose a reason for hiding this comment

0xApotheosis Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

woodenfurniture Jan 25, 2024

Choose a reason for hiding this comment

gomesalexandre Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

gomesalexandre Jan 25, 2024

Choose a reason for hiding this comment

woodenfurniture Jan 25, 2024

Choose a reason for hiding this comment

woodenfurniture left a comment

Choose a reason for hiding this comment

gomesalexandre commented Feb 2, 2024

gomesalexandre commented Feb 2, 2024

0xApotheosis left a comment • edited Loading

Choose a reason for hiding this comment

0xApotheosis Feb 4, 2024

Choose a reason for hiding this comment

gomesalexandre Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

0xApotheosis left a comment

Choose a reason for hiding this comment

0xApotheosis left a comment

Choose a reason for hiding this comment

0xApotheosis left a comment

Choose a reason for hiding this comment

gomesalexandre commented Jan 24, 2024 •

edited

Loading

gomesalexandre commented Jan 24, 2024 •

edited

Loading

0xApotheosis Feb 2, 2024 •

edited

Loading

gomesalexandre Jan 25, 2024 •

edited

Loading

0xApotheosis left a comment •

edited

Loading

gomesalexandre Feb 5, 2024 •

edited

Loading