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

As a trader, I need to swap one token for another #5073

Closed
5 of 6 tasks
rowgraus opened this issue Apr 11, 2022 · 11 comments
Closed
5 of 6 tasks

As a trader, I need to swap one token for another #5073

rowgraus opened this issue Apr 11, 2022 · 11 comments
Assignees
Labels
AMM Inter-protocol Overarching Inter Protocol user-story User story that needs to be implemented and tested.

Comments

@rowgraus
Copy link

rowgraus commented Apr 11, 2022

  • I must be quoted a price.
  • I would like to know how much my trade will impact the price in the pool (“Price Impact”)
  • I must be able to set a maximum slippage % (“Slippage”) which will allow my trade to process even if the price moves due to other traders prior to my transaction being accepted
    • up to 5% is supported; is more than that needed?
  • Note: “Minimum received” or “Maximum paid” should also be shown which is a price quote at the worst possible slippage result
@rowgraus rowgraus added the user-story User story that needs to be implemented and tested. label Apr 11, 2022
@Tartuffo Tartuffo added the Inter-protocol Overarching Inter Protocol label Apr 11, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Apr 12, 2022
@samsiegart
Copy link
Contributor

Related:
image

@samsiegart
Copy link
Contributor

I would like that price to be as-real-time as possible (i.e., should update based on notifications)

Looks like the dapp doesn't currently do this, it just loads the price when you select an asset. It could be annoying when you're entering a number and the rate constantly updates and changes your inputs, so my proposal is to debounce the price update until after you stop typing for a certain duration. Then, update whichever input that you didn't most recently type into to reflect the new exchange rate

@samsiegart
Copy link
Contributor

Looks like slippage is a bit buggy as well. It doesn't update the "receive at least" when you modify it.

@dckc
Copy link
Member

dckc commented May 4, 2022

I can't get a swap to go through. My offer is accepted, and the dapp says "Assets successfully swapped" but my purse balances don't change (except for some RUN fees).

Screenshot at 2022-05-03 19-05-51
Screenshot at 2022-05-03 19-05-59

@dckc
Copy link
Member

dckc commented May 4, 2022

@samsiegart wrote:

I was just able to swap 0.05 RUN for 0.049804 AUSD, leading me to believe it's something to do with prices changing for larger swaps

After I added 25 RUN / 25 USDC of liquidity, I was able to swap 1 RUN for 1 USDC with 5% slippage tolerance.

I would close this except that I don't see an UI for this part:

  • I would like to know how much my trade will impact the price in the pool

@dckc
Copy link
Member

dckc commented May 6, 2022

@rowgraus , as noted above, @samsiegart and I both tested basic "I need to swap one token for another" stories. I presume the "I would like to know how much my trade will impact the price in the pool" part is not critical.

To make it easier to see that not all of the details in the description have been tested, I made it into a checklist and checked the parts that we tested. In at least one case (#5019) the unfinished parts are tracked in other issues. Feel free to open issues for the other sub-items... or if you prefer, go ahead an re-open this.

@dckc
Copy link
Member

dckc commented Aug 10, 2022

re-opening for smart wallet integration

@dckc
Copy link
Member

dckc commented Aug 18, 2022

hey! it can be done!

some rough edges:

  • when bridge disconnects, dApp spams requests to localStorage
  • connection component has poor feedback around when user has to do what
  • wallet follower "bad proof"
  • instance shows up as unnamed after you accept the offer (perhaps should do suggestInstance)
  • suggestIssuer doesn't show when your request is coming
  • when you rename the petname, you have to press enter; clicking the button doesn't result in saving it
  • in local.agoric.com, what you have to enter isn't clear trailing slash wallet.agoric.app/ or /wallet/ won't work.

one user's journey:
starting from nothing: no keplr, ...

  • set up account in keplr
    • googled keplr... chrome store... Add extension... (Chrome offers to sync; declined) reads keplr overview
    • brings up keplr extension... how to create account? googles...
    • clue: wallet.agoric.app clue: keplr instructions. clue: pinning a chrome extension
    • chooses account name, password... confirms seed phrase...
  • goes to instagoric faucet... provisions smart wallet account
  • goes to wallet.agoric.app... approves keplr pop-up chooses web wallet. connect smartwallet
    • chain config url widget requires an extra click
    • accepts keplr prompt
    • sees 25 IST. (notes it's different from 50 or so with ag-solo)
  • dapp approval
    • goes to amm.agoric.app - "enable the dapp" prompt covered "wallet" button
      • clue: what to put in local.agoric.app
    • tried reloading the dapp, then wallet... discovered "signal wallet" button... tried it...
    • approved dapp in wallet
      • tried to rename dapp petname (appeared to work)
  • trade
    • selected IST, purse, 5 IST, atom purse

      • slippage warning
    • please approve offer in your wallet...

    • keplr popped up... studied the tx gobbldygook... approved signing

    • watched balances... noted offer card state change

    • "allright" 0.25 IbcATOM swapped!

    • traded again... up to 0.49

      • "did not match proof" (took screenshot)
    • started looking at adding liquidity...

    • chain config url widget requires an extra click

    • "did not match proof" (took screenshot)

@dckc dckc closed this as completed Aug 18, 2022
@dckc
Copy link
Member

dckc commented Nov 1, 2022

I think changes to the smart wallet API mean this doesn't quite work any more.

@turadg
Copy link
Member

turadg commented Sep 6, 2023

I think this is no longer planned. If it is, @otoole-brendan please re-open and update the body with what's left to do.

@turadg turadg closed this as not planned Won't fix, can't repro, duplicate, stale Sep 6, 2023
@otoole-brendan
Copy link
Contributor

I think you're right @turadg. cc @rowgraus in case you disagree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMM Inter-protocol Overarching Inter Protocol user-story User story that needs to be implemented and tested.
Projects
None yet
Development

No branches or pull requests

7 participants