fix: synchronize buyAssetAccountId to sellAssetAccountNumber on swapper mount #6124
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| // currently, a multi-hop trade will be automagically routed though the accountId corresponding to | ||
| // the accountNumber for the first hop. |
There was a problem hiding this comment.
That was the intent, but wasn't working as intended - comment reworded and moved on top of buyAssetAccountId
| sellAssetAccountId: initialSellAssetAccountId, | ||
| buyAssetAccountId: initialBuyAssetAccountId, |
There was a problem hiding this comment.
Technically no only defaults, but making it clear that the main intent of these is to get initial values - else if all we cared about was selecting the buy/sell AccountIds, we could directly use the selectors that are consumed there
| // - if this was to fail for any reason, we default to the first account number as a default | ||
| const buyAssetAccountId = useAppSelector(state => | ||
| selectLastHopBuyAccountId(state, { accountId: maybeMatchingBuyAccountId }), | ||
| ) |
There was a problem hiding this comment.
I'd be inclined to move all of this business logic into a new selector selectLastHopDefaultBuyAccountId (or similar, better name) which means the concept of maybe passing in a filter that is assumed to be maybeMatchingBuyAccountId is no longer required.
i.e revert the original selectLastHopBuyAccountId and create a new selector encapsulating all of the above business logic.
There was a problem hiding this comment.
That was the original intent and would be much nicer, unfortunately this isn't possible because of the different arities of selectors, and the fact we can't use a selector's return value as input for the next one
|
Checked in with @gomesalexandre, confirmed we can dismiss the current blocking comment from @woodenfurniture as we cannot implement it at this time (see #6124 (comment)). |
Description
Does what it says on the box - when the swapper originally mounts, the
buyAssetAccountIdshould be synchronized with thesellAssetAccountId's account number, if there is a matching account on that index on the buy side.Pull Request Type
Issue (if applicable)
closes #6121
Risk
Theoretically low, effectively medium in terms of domain touched. While this technically brings back the behavior that was intended (see the removed/reworded comment), if we got this wrong, there is a risk of funds ending up in the wrong account
None
Testing
Have a balance of buy asset that is higher on any 1+ account than on your 0th account
On swapper original mount, ensure that the buy account number is matching the sell account number
Ensure it is still possible to choose another account, both on the buy and the sell side, and that they do not synchronize after the original mount and synchronization
Play around with account selection and ensure you're still getting quotes
Ensure funds are landing in the right account
Engineering
Operations
Screenshots (if applicable)
Default buy account
develop.mov
this.diff.mov
Quotes are happy, and funds are ending in the correct address
correct.account.mov