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

[WIP] Atomic trade #4414

Closed
wants to merge 12 commits into from
Closed

[WIP] Atomic trade #4414

wants to merge 12 commits into from

Conversation

sqrrm
Copy link
Member

@sqrrm sqrrm commented Aug 17, 2020

This PR implements a new account type, Atomic BSQ with a separate trade protocol, see bisq-network/proposals#50

Atomic trade protocol is completed in one request -> response interaction between taker and maker.

  1. Taker sends a CreateAtomicTxRequest to maker. This includes all the data needed to create and sign the atomic tx from maker's side.
  2. Maker verifies all inputs and answers with CreateAtomicTxResponse that includes an atomic tx with maker's inputs signed.
  3. Taker verifies the inputs and that their own outputs are paid as expected, signs and publishes the completed atomic tx.
  4. Fin

image
There is still a minimum 15% deposit to avoid messing around too much with current trade protocol. It's not actually needed since the trade is an atomic transaction, but I leave that as a future improvement.

image
Can't be taken by an old client since the account type doesn't exist

image
Take with new client, requires to have the deposit, but it's not used apart from checking that it's available during the take offer process.

image
Trade completes immediately.

image
image
Shows in transactions as atomic

image
image
Need a confirmation before it can show the type of BSQ tx

image
image
After confirmation

Move donation address validation to its own class. Will be needed for
atomic transaction validation.
Add a hidden account type that's automatically used for BSQ trades. This
means old orders not using AtomicAccount can still complete the trades
as per normal and new offers placed with AtomicAccount can be taken by
anyone with an upgraded client, but won't be possible to take by users
with older clients.

The atomic account is added on startup if not already added. There is no
data associated with the account, a new BSQ address will be chosen
automatically during the atomic trade process.
Refactor signTx and allow for signing of only some inputs
Atomic trade protocol is completed in one request -> response
interaction between taker and maker.
1. Taker sends a CreateAtomicTxRequest to maker. This includes all the
data needed to create and sign the atomic tx from maker's side.
1. Maker verifies all inputs and answers with CreateAtomicTxResponse
that includes an atomic tx with maker's inputs signed.
1. Taker verifies the inputs and that their own outputs are paid as
expected, signs and publishes the completed atomic tx.
1. Fin

There is only one TakerVerifies and one MakerVerifies task as the buy
and sell side are very similar, the main difference lies in whether the
actor is taker or maker.

It's currently not possible to verify BTC inputs, but bad inputs (such
as spent ones) would generate a tx that won't broadcast. No funds can be
lost as the outputs are verified on both sides.
Show atomic trades as such in transaction views, both BTC and BSQ
transaction view.
@sqrrm sqrrm closed this Aug 19, 2020
@sqrrm sqrrm reopened this Aug 19, 2020
@sqrrm
Copy link
Member Author

sqrrm commented Aug 24, 2020

When the makerAsBuyer (selling BSQ) doesn't have enough BSQ at the time their offer is taken they get a warning message popup
image

The taker gets an error popup
image

This could be handled better. First, a hold on BSQ could be added to make sure the offer is funded while it's available. Then there is old time out error that's not very pretty. We could improve on the message and make it look a bit less threatening, but the effect is still going to be a failed take offer attempt no matter how we spin it.

@sqrrm sqrrm changed the title Atomic trade [WIP] Atomic trade Sep 1, 2020
@stale
Copy link

stale bot commented Oct 2, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@flix1
Copy link
Member

flix1 commented Sep 14, 2021

This looks amazing. Looking forward to seeing it implemented!

@pazza83
Copy link

pazza83 commented Sep 19, 2021

This is great thanks for this. Looking forward to try it out.

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

Successfully merging this pull request may close these issues.

None yet

4 participants