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

Tell the user about the swap limits #255

Closed
thomaseizinger opened this issue Mar 2, 2021 · 6 comments · Fixed by #268
Closed

Tell the user about the swap limits #255

thomaseizinger opened this issue Mar 2, 2021 · 6 comments · Fixed by #268

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 2, 2021

The ASB currently limits the amount we can swap to a specific XMR value.

We should tell the user a max amount of BTC that we can swap while we are waiting for their funds.

We should also display the rate to them and potentially a timeout for how long this rate is valid. To do that, we need more information from the ASB about the rate before the swap even starts.

Suggestion:

  1. Add a protocol to the ASB that allows to query the latest price + a limit (+ optionally a timeout).
  2. When waiting for funds from the user on the CLI, display this information (+ optionally abort if we hit the timeout).
  3. Swap everything the user transfers or the limit if they transfer more.
@thomaseizinger thomaseizinger created this issue from a note in Add stability, recovery and analytics (Weekly sprint backlog) Mar 2, 2021
@da-kami
Copy link
Member

da-kami commented Mar 2, 2021

In my understanding we touch three different topics in this issue:

  1. The maximum tradable amount and currency allowed by the ASB
  2. The possibility of getting a price (or quote) without triggering a swap
  3. The time until an ASB offer is "valid"

I don't think we should invest time into 3. because it is a rabbit hole.

I see the biggest problem at our execution_setup, that is tightly coupled to quote request/response.
This was done, because the ASB is dictating the price.

I am not sure we need an entirely new protocol to achieve 1. and 2. - but we will have to separate the swap_execution trigger from the quote request/response.

I think the CLI should be able to do a quote request and just get a response without any execution setup.
Additionally the CLI should be able to trigger the execution setup, which implies the quote at the time of triggering the execution setup (i.e. the quote might be different).

Concerning 1. I think it would be easier to change the ASB's maximum amount acceptable to BTC. By doing so we take the problem of calculating "what is allowed" away from the user, but then have the problem that the ASB has to decide if a trade of max BTC can be fulfilled with the current XMR balance.
Since we don't have an elaborate strategy for making sure we can fulfill trades (i.e. bookkeeping like in the old Nectar maker) I would not invest too much time in making this work perfectly - we can have a reasonable buffer so that when triggering swap execution we can be confident the ASB can fulfill the XMR funding. Additionally notification to add funds if we use half the balance (or similar).

@thomaseizinger thomaseizinger self-assigned this Mar 2, 2021
@thomaseizinger thomaseizinger moved this from Weekly sprint backlog to In progress in Add stability, recovery and analytics Mar 2, 2021
@thomaseizinger
Copy link
Contributor Author

I think the CLI should be able to do a quote request and just get a response without any execution setup.

Correct. The problem is that the ASB already kicks of a new swap as part of the quote request.
I think we should detach this such that the quote behaviour just replies with a quote, but doesn't change the state otherwise.

In order to make the rest of the application work, we would need to either:

  • extend execution setup with an additional message where the ASB tells the CLI, what the exact amount for the swap will be, which means effectively merging the current quote behaviour into the execution setup (which should then probably be renamed)
  • have the CLI send over both amounts with a validation on the ASB side

What I don't like about it is that it is not longer strictly "execution setup". Thoughts?

Concerning 1. I think it would be easier to change the ASB's maximum amount acceptable to BTC. By doing so we take the problem of calculating "what is allowed" away from the user, but then have the problem that the ASB has to decide if a trade of max BTC can be fulfilled with the current XMR balance.

Agreed. It shouldn't be too hard to validate, whether the XMR amount exceeds the current balance.

@rishflab
Copy link
Member

rishflab commented Mar 2, 2021

I would decouple the EventLoop from the Swap so the EventLoop can be initialised earlier. Then you can add and use the RateRequest behaviour to get the prelimary rate to display to the user on startup.

  1. The time until an ASB offer is "valid"

I don't think we should invest time into 3. because it is a rabbit hole.

Agreed.

@thomaseizinger
Copy link
Contributor Author

Okay, here is my progress:

I managed to extract a nice "quote" behaviour that allows Bob to query quotes from Alice and he gets a price + a max quantity. That is what we need on the CLI in order to display it to the user.

Several open questions:

  1. How do we trigger this behaviour? Currently the eventloop is instantiated after we've received the money from the user but I would need the quote already before that. This sounds like I need to decouple the swarm from the eventloop and instantiate it separately so that I can use it before already (is this what you meant @rishflab ?)

  2. The execution setup assumes two things: That both amounts are present AND that Alice knows that Bob actually wants to setup a swap (the async-await-behaviour needs to be setup explicitely). At the moment, the quote behaviour solves both of these problems so I think we need to retain that functionality, as ugly as it is :)

thomaseizinger added a commit that referenced this issue Mar 3, 2021
Instead of instantiating the `EventLoop` within the builder, we only
pass in the necessary arguments (which is the `EventLoopHandle`) to
the Builder upon `new`.

This is work towards #255 which will require us to perform network
communication (which implies having the `EventLoop`) before starting
a swap.
@rishflab
Copy link
Member

rishflab commented Mar 3, 2021

  1. How do we trigger this behaviour? Currently the eventloop is instantiated after we've received the money from the user but I would need the quote already before that. This sounds like I need to decouple the swarm from the eventloop and instantiate it separately so that I can use it before already (is this what you meant @rishflab ?)
    yep
  1. The execution setup assumes two things: That both amounts are present AND that Alice knows that Bob actually wants to setup a swap (the async-await-behaviour needs to be setup explicitely). At the moment, the quote behaviour solves both of these problems so I think we need to retain that functionality, as ugly as it is :)

I think the execution setup will remain the same. The new "quote" behaviour will just be used to get data to feed to our existing behaviour which I think should remain unchanged.

thomaseizinger added a commit that referenced this issue Mar 3, 2021
Instead of instantiating the `EventLoop` within the builder, we only
pass in the necessary arguments (which is the `EventLoopHandle`) to
the Builder upon `new`.

This is work towards #255 which will require us to perform network
communication (which implies having the `EventLoop`) before starting
a swap.
@thomaseizinger
Copy link
Contributor Author

  1. How do we trigger this behaviour? Currently the eventloop is instantiated after we've received the money from the user but I would need the quote already before that. This sounds like I need to decouple the swarm from the eventloop and instantiate it separately so that I can use it before already (is this what you meant @rishflab ?)
    yep
  1. The execution setup assumes two things: That both amounts are present AND that Alice knows that Bob actually wants to setup a swap (the async-await-behaviour needs to be setup explicitely). At the moment, the quote behaviour solves both of these problems so I think we need to retain that functionality, as ugly as it is :)

I think the execution setup will remain the same. The new "quote" behaviour will just be used to get data to feed to our existing behaviour which I think should remain unchanged.

Yep, @da-kami and I discussed this and came to the following conclusion:

  1. Introduce a new quote behaviour that returns price and max-quantity.
  2. Adapt the existing quote behaviour by changing it into a "spot-price" behaviour because that is what it semantically really is.
  3. Once Untangle Bob's EventLoop from the Builder #264 is merged, we can use the new quote behaviour before we ask the user to deposit funds and can display them the limit and rate right away.

bors bot added a commit that referenced this issue Mar 3, 2021
264: Untangle Bob's `EventLoop` from the `Builder` r=thomaseizinger a=thomaseizinger

This is work towards #255.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
thomaseizinger added a commit that referenced this issue Mar 3, 2021
Instead of instantiating the `EventLoop` within the builder, we only
pass in the necessary arguments (which is the `EventLoopHandle`) to
the Builder upon `new`.

This is work towards #255 which will require us to perform network
communication (which implies having the `EventLoop`) before starting
a swap.
bors bot added a commit that referenced this issue Mar 3, 2021
257: Allow ASB to be configured with max BTC buy amount r=thomaseizinger a=thomaseizinger

This will make it easier to also configure the CLI to display an appropriate max amount the user has to deal with.

This is the first step in working towards #255.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
thomaseizinger added a commit that referenced this issue Mar 3, 2021
Previously, the user neither knew the price nor the maximum quantity
they could trade. We now request a quote from the user and display
it to them.

Fixes #255.
thomaseizinger added a commit that referenced this issue Mar 3, 2021
Previously, the user neither knew the price nor the maximum quantity
they could trade. We now request a quote from the user and display
it to them.

Fixes #255.
bors bot added a commit that referenced this issue Mar 4, 2021
268: Introduce `quote` protocol and display it to the user before they fund r=thomaseizinger a=thomaseizinger

Fixes #255.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@bors bors bot closed this as completed in 601bf07 Mar 4, 2021
Add stability, recovery and analytics automation moved this from In progress to Done Mar 4, 2021
abraham-nixon added a commit to abraham-nixon/xmr-btc-swap that referenced this issue Feb 15, 2022
257: Allow ASB to be configured with max BTC buy amount r=thomaseizinger a=thomaseizinger

This will make it easier to also configure the CLI to display an appropriate max amount the user has to deal with.

This is the first step in working towards comit-network/xmr-btc-swap#255.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants