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

Swapper sell and buy accounts should synchronize by default #6121

Closed
gomesalexandre opened this issue Feb 2, 2024 · 1 comment · Fixed by #6124
Closed

Swapper sell and buy accounts should synchronize by default #6121

gomesalexandre opened this issue Feb 2, 2024 · 1 comment · Fixed by #6124
Assignees

Comments

@gomesalexandre
Copy link
Contributor

gomesalexandre commented Feb 2, 2024

Overview

Spotted in #6072, pasted report below.

There is a possibly undesirable regression here, where on mount the recipient address is set by default to be same as the auto-selected address with the highest balance. This is expected.

Very interesting find @0xApotheosis. Unfortunately, this isn't a regression and is a current prod bug. Summarizing the findings below, but moving this to draft to tackle it as an underlying PR 🙏🏽

Current develop and prod, as well as this diff wrongly do cross-account as a default if you happened to have different highest asset accounts on the buy and sell side. The receive address isn't set to be the same as the send address, but rather, both automagically select highest receive address, which doesn't ensure synchronization.

  • Prod
image image
  • Develop
image image
  • This diff

The above screenshots are with the very default ETH-FOX trade route on mount, without inputting or changing anything. Highest balance on the sell-side is expected, but pretty sure that shouldn't automagically happen on the receive side as we don't want cross-account as a default, which could be the case, and is here.

The regression is that now that when the from account is changed, the recipient address does not update to match it, so the user ends up inadvertently quoting for, and potentially executing, a cross-account swap.

This unfortunately isn't happening on account change - account change will only change your account on a given side, but doesn't automagically synchronize accounts.

References and additional details

N/A

Acceptance Criteria

  • The base expected behavior here is the sell asset account should be the only one automagically set to be the highest balance account. The buy account should not do this as a default, and should initially synchronize to the same account as the sell account, to ensure no cross-account trade is happening as a default.

In other words, on mount, the active accounts are defined by the highest balance sell account, meaning the buy account will always be the same as the highest balance sell one on mount.

The second part (on account change) of the acceptation criteria would be a @shapeshift/product question, gm @reallybeard @twblack88. We have two options:

  1. We can either initially set one to be the same as the other on account change (no matter whether it was the buy or sell account number which was changed), obviously with the option to then change it. This may get very very tricky to implement, as we would most likely end up in an infinite loop of e.g
  • account 1 highest on sell, meaning account 1 is both autoselected on the sell and buy side
  • I decide to change the buy account to be account 0, both become account 0, but I can still make the sell side account 1 if I wish to, to perform a cross-account trade
  • changing the sell side to account 1 would synchronize both to 1 again, so we would have to do some dodgy checks to see if the opposite account was just explicitly changed by the user, to let them change it again
  1. Or, as a safer option that's realistic to implement, only synchronize accounts initially, but don't try to do any synchronization later on. In other words, as long as you start changing accounts, you explicitly expressed intent to manually choose your accounts, and we shouldn't try to hijack this and decide what you want as a user with some arbitrary heuristics.
  • There is probably some discussion that needs to be done on what constitutes a "by default" as well.
    Is the default first mount of swapper only? What if I change pairs to same-chains pairs e.g ETH->FOX to CRV -> USDC, should that be considered a new default? What if I change pairs to be different chains one e.g DOGE -> ATOM ?

Need By Date

No response

Screenshots/Mockups

No response

Estimated effort

No response

@twblack88
Copy link
Contributor

aligned with option 2 far more user control and more sane.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants