Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

Taker's order criteria #89

Closed
D4nte opened this issue Jan 17, 2020 · 4 comments · Fixed by #116
Closed

Taker's order criteria #89

D4nte opened this issue Jan 17, 2020 · 4 comments · Fixed by #116
Assignees

Comments

@D4nte
Copy link

D4nte commented Jan 17, 2020

Problem

To get an order, the taker needs to go through several hoops:

  1. Get orders from maker by doing TakerNegotiator.getOrderByTradingPair("ETH-BTC")
  2. Do manual sanity checks on the order: https://github.com/comit-network/create-comit-app/blob/e2718a60bf7319e2504bb355c0406b00da0ae055/create/new_project/examples/separate_apps/src/taker.ts#L49
  3. Manually check that the rate is acceptable: https://github.com/comit-network/create-comit-app/blob/e2718a60bf7319e2504bb355c0406b00da0ae055/create/new_project/examples/separate_apps/src/taker.ts#L67
  4. Take the order using TakerNegotiator.takeOrder(order): https://github.com/comit-network/create-comit-app/blob/e2718a60bf7319e2504bb355c0406b00da0ae055/create/new_project/examples/separate_apps/src/taker.ts#L73

The downsides are:

  • The taker needs to specify the criteria in several steps (first pair, then check itself)
  • Validation of the order is manual, a dev would expect to be able to do Order.validate() or even only return valid orders from the TakerNegotiator.getOrderByTradingPair call by filtering
  • The filtering is quite manual and forces the dev to work through the properties of the order

Goal

Provide a straightforward, function based, API to the developer. Alleviate the work.

Recommendation

Re-think the API describe above to simplify it.
A better flow would be:

const criteria = {
	buy: "BTC",
	sell: "ETH",
	btcAmount: 0.5,
	allowOffersWithLesserAmount: false,
	maximumRate: 10
}

// Probably revise the `TakerNegotiator` terminology
const order = await takerNegotiator.getOffer(criteria);

if (!order.isValid()) { // This is an optional call and could/should be done already by `getOffer`
	// Ban this maker
}

if (!order.matches()) {  // This is an optional call and could/should be done already by `getOffer`
	// Tell the user why we cannot find an appropriate order
	console.log("Order found but does not match criteria", order.matchFailReason());
	// -> "Order found but does not match criteria { reason: 'maximumRate exceeded', expected: 10, actual: 15 }"
	// -> "Order found but does not match criteria { reason: 'lesser amount provided and not allowed', expected: 0.5, actual: 0.1 }"
}

const swap = await order.take();
@D4nte D4nte added the to-refine Clarifications are needed label Jan 17, 2020
@D4nte
Copy link
Author

D4nte commented Jan 17, 2020

@yosriady what do you think about the proposed flow?

@yosriady
Copy link

@D4nte

Looks better! The core idea is to automatically give users a pre-filtered list of acceptable orders to take from.

const order = await takerNegotiator.getOffer(criteria);

What's an 'offer'? Would this function return one or a list of valid offers?

@bonomat
Copy link
Member

bonomat commented Jan 19, 2020

const criteria = {
  buy: "BTC",
  sell: "ETH",
  btcAmount: 0.5,
  allowOffersWithLesserAmount: false,
  maximumRate: 10
}
// Probably revise the `TakerNegotiator` terminology
const order = await takerNegotiator.getOffer(criteria);
...

Are these fields optional?

//Ignore what was written here before :) //

@D4nte
Copy link
Author

D4nte commented Jan 19, 2020

const criteria = {
  buy: "BTC",
  sell: "ETH",
  btcAmount: 0.5,
  allowOffersWithLesserAmount: false,
  maximumRate: 10
}
// Probably revise the `TakerNegotiator` terminology
const order = await takerNegotiator.getOffer(criteria);
...

Are these fields optional?

//Ignore what was written here before :) //

Some field are likely to be optional while other are mandatory.

@D4nte D4nte removed the to-refine Clarifications are needed label Jan 19, 2020
@D4nte D4nte removed their assignment Jan 19, 2020
@D4nte D4nte added this to the Sprint 28 ❤💸 milestone Feb 14, 2020
@D4nte D4nte self-assigned this Feb 14, 2020
bors bot added a commit to comit-network/create-comit-app that referenced this issue Feb 20, 2020
440: Use new `TakerNegotiator` interface r=D4nte a=D4nte

Incorporates SDK changes from comit-network/comit-js-sdk#116

Related to comit-network/comit-js-sdk#89

Co-authored-by: Franck Royer <franck@coblox.tech>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants