Improve `TakerNegotiator` interface #116

D4nte · 2020-02-14T11:12:00Z

This test demonstrates how to use the TakerNegotiator.
Note that the last part await order.take() is not tested as it involves creating a few mock. Not yet sure whether I should do it.

comit-js-sdk/src/negotiation/taker_negotiator.spec.ts

Line 25 in f6d5cf0

it("Returns an order and takes it", async () => {

Usage in the example drafted here: comit-network/create-comit-app#440

Resolves #89

bonomat

If I see it correctly then you had to make bitcoinWallet and ethereumWallet optional and expose these two fields publicly just to facilitate the tests.
I think we should try to avoid this.

src/actor.ts

src/negotiation/order.ts

src/negotiation/taker_negotiator.spec.ts

D4nte · 2020-02-15T05:54:55Z

If I see it correctly then you had to make bitcoinWallet and ethereumWallet optional and expose these two fields publicly just to facilitate the tests.
I think we should try to avoid this.

We won't need a Bitcoin Wallet if ones only wants to do LNBTC-ERC20 swaps. Hence I believe it is a move in the right direction.

bonomat · 2020-02-15T07:46:22Z

We won't need a Bitcoin Wallet if ones only wants to do LNBTC-ERC20 swaps. Hence I believe it is a move in the right direction.

Ok, valid point.

bonomat · 2020-02-16T23:41:18Z

src/actor.ts

-  const comitClient = new ComitClient(bitcoinWallet, ethereumWallet, cnd);
+  const comitClient = new ComitClient(cnd);
+  comitClient.ethereumWallet = ethereumWallet;
+  comitClient.bitcoinWallet = bitcoinWallet;


Instead of exposing these fields publicly you could apply the builder pattern. This would allow for something like this:

const comitClient = new ComitClient(cnd) .withEthereumWallet(ethereumWallet) .withBitcoinWallet(bitcoinWallet) ....;

Let me know what you think and I can provide a small PR once your changes are in.

Please hold, I am finishing off few things. I'll ping you.

da-kami · 2020-02-17T02:22:45Z

src/negotiation/order.ts

 import { getToken, Token } from "../tokens/tokens";

-export interface Order {
-  tradingPair: string;
+export interface OrderParams {


🙃 I find the design of the Order a bit confusing. From code

export class Order { constructor( public readonly orderParams: OrderParams, public readonly criteria: TakerCriteria, public readonly takeOrder: ( orderParams: OrderParams ) => Promise<Swap | undefined> ) {}

So the order consists of OrderParams and a TakerCriteria, why is the abstraction of OrderParams needed? What exactly is a TakerCriteria?

I feel there is quite a lot of classes now and it's not immediately understandable why and what they are for.
Motivation for writing this: Started to write comments for code documentation and it's not that easy to explain what is going on. What is currently there works, but I wonder if we could design it so it is easier to understand.

Can we simplify this by integrating the OrderParams into the Order? Is this abstraction really needed?

Please note that the lib consumer is not supposed to constructor Order by themselves.
They would only use OrderParams (a simple object) to add orders to the order book.

Interesting point about documentation because this means that the constructor of Order should not be documented for example.

I think I understand what you are doing, but I still find the naming a bit confusing. I don't have a perfect solution in mind yet, but maybe we should step back, take a whiteboard and design this outside the code. The way Order and OrderParams is introduced in this PR is a bit confusing for me. we have a lot of variables named order now, that are actually OrderParams. Maybe a simple rename can fix this, not sure.

da-kami · 2020-02-17T02:24:54Z

src/negotiation/maker_client.ts

+    return response.data;
+  }
+
+  public async takeOrder(order: OrderParams, swapId: string) {


❓ How come we already have a swapId at this point? Aren't we supposed to get the swapId from the maker after taking the order?

(Maybe I'm just confused by the syntax...?)

This is the swap Id indeed. I have documented in f622aea

da-kami · 2020-02-17T02:27:47Z

Overall: Like the approach towards tests, but don't understand why certain design decisions were made. Would be cool if could go over this with me Franck and explain it (Would help for creating good documentation).

D4nte · 2020-02-17T05:28:41Z

Overall: Like the approach towards tests, but don't understand why certain design decisions were made. Would be cool if could go over this with me Franck and explain it (Would help for creating good documentation).

Please review @da-kami I added documentation.

da-kami

Sry for being a bit nit-picky about this @D4nte - added some comments.

src/negotiation/maker_client.ts

src/negotiation/maker_negotiator.ts

src/negotiation/order.ts

src/negotiation/taker_negotiator.spec.ts

src/negotiation/taker_negotiator.ts

da-kami

added one commit.

da-kami · 2020-02-18T01:03:05Z

src/negotiation/order.ts

+ */
+export class Order {
+  /**
+   * **Note: This should not be used, `Order` should be created by using `TakerNegotiatior.getOrder()`


Suggested change

* **Note: This should not be used, `Order` should be created by using `TakerNegotiatior.getOrder()`

Not a blocker, feel free to ignore.

Not only this helps with mocking but I believe it is the correct move. Indeed, if one decides to use the SDK for LN-Bitcoin to ERC20 only, then It would not use a `BitcoinWallet` class but one that handles LightningBitcoin

Create mock class to mock axios calls.

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>

mergify · 2020-02-24T23:32:29Z

bors r+

bors · 2020-02-24T23:32:30Z

Already running a review

Fix describe

9588aeb

github-actions bot assigned D4nte Feb 14, 2020

D4nte requested review from bonomat, da-kami and yosriady February 14, 2020 11:12

bonomat suggested changes Feb 15, 2020

View reviewed changes

bonomat approved these changes Feb 15, 2020

View reviewed changes

D4nte added the no-mergify Stop mergify to merge this automatically label Feb 15, 2020

bonomat reviewed Feb 16, 2020

View reviewed changes

da-kami reviewed Feb 17, 2020

View reviewed changes

D4nte requested a review from da-kami February 17, 2020 05:28

da-kami suggested changes Feb 17, 2020

View reviewed changes

D4nte force-pushed the 89-taker-order-criteria branch from 584e472 to ec07812 Compare February 17, 2020 23:08

D4nte mentioned this pull request Feb 17, 2020

Use new TakerNegotiator interface comit-network/create-comit-app#440

Merged

D4nte requested a review from luckysori February 17, 2020 23:21

D4nte added no-mergify Stop mergify to merge this automatically and removed no-mergify Stop mergify to merge this automatically labels Feb 17, 2020

D4nte requested a review from da-kami February 17, 2020 23:22

da-kami approved these changes Feb 18, 2020

View reviewed changes

Franck Royer added 6 commits February 18, 2020 13:23

Do not include /dists in jest path

60db32f

Extract MakerClient to own file

8522192

Remove wallets from ComitClient constructor

e012ad2

Not only this helps with mocking but I believe it is the correct move. Indeed, if one decides to use the SDK for LN-Bitcoin to ERC20 only, then It would not use a `BitcoinWallet` class but one that handles LightningBitcoin

Add tests for TakerNegotiator

d49d06e

Create mock class to mock axios calls.

Introduce Order class

7c1cdf4

Use ledgers and assets as trading pair, remove dupe info

179c0d1

Franck Royer and others added 10 commits February 18, 2020 13:26

getOrder now returns an Order class instance

9e73c8e

Add Order.take()

371ee1e

Add comment on Order

bf284f9

Validate order before adding it to the orderbook

9e08054

Add rate matching

9377dda

Add comments on orderSwapMatchesForMaker

b54088d

Remove @description tag

0720f52

Improve comments

2c61232

Rename order to orderParams where relevant

c437ae5

remove Notes, make execAndTakeOrder private

2077cd3

D4nte force-pushed the 89-taker-order-criteria branch from dd03f68 to 2077cd3 Compare February 18, 2020 02:31

D4nte merged commit 2cbf2f4 into master Feb 18, 2020

D4nte deleted the 89-taker-order-criteria branch February 18, 2020 02:33

D4nte removed the no-mergify Stop mergify to merge this automatically label Feb 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `TakerNegotiator` interface #116

Improve `TakerNegotiator` interface #116

D4nte commented Feb 14, 2020 •

edited

Loading

bonomat left a comment

D4nte commented Feb 15, 2020

bonomat commented Feb 15, 2020

bonomat Feb 16, 2020

D4nte Feb 17, 2020

da-kami Feb 17, 2020 •

edited

Loading

D4nte Feb 17, 2020

da-kami Feb 17, 2020

da-kami Feb 17, 2020 •

edited

Loading

D4nte Feb 17, 2020

da-kami commented Feb 17, 2020

D4nte commented Feb 17, 2020

da-kami left a comment

da-kami left a comment

da-kami Feb 18, 2020

mergify bot commented Feb 24, 2020

bors bot commented Feb 24, 2020

Improve TakerNegotiator interface #116

Improve TakerNegotiator interface #116

Conversation

D4nte commented Feb 14, 2020 • edited Loading

bonomat left a comment

Choose a reason for hiding this comment

D4nte commented Feb 15, 2020

bonomat commented Feb 15, 2020

bonomat Feb 16, 2020

Choose a reason for hiding this comment

D4nte Feb 17, 2020

Choose a reason for hiding this comment

da-kami Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

D4nte Feb 17, 2020

Choose a reason for hiding this comment

da-kami Feb 17, 2020

Choose a reason for hiding this comment

da-kami Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

D4nte Feb 17, 2020

Choose a reason for hiding this comment

da-kami commented Feb 17, 2020

D4nte commented Feb 17, 2020

da-kami left a comment

Choose a reason for hiding this comment

da-kami left a comment

Choose a reason for hiding this comment

da-kami Feb 18, 2020

Choose a reason for hiding this comment

mergify bot commented Feb 24, 2020

bors bot commented Feb 24, 2020

Improve `TakerNegotiator` interface #116

Improve `TakerNegotiator` interface #116

D4nte commented Feb 14, 2020 •

edited

Loading

da-kami Feb 17, 2020 •

edited

Loading

da-kami Feb 17, 2020 •

edited

Loading