feat: quotes-server #430

bodymindarts · 2023-09-13T13:51:58Z

No description provided.

nicolasburtey · 2023-09-13T15:17:35Z

proto/quotes/quote_service.proto

+
+message GetQuoteToBuyCentsRequest {
+  oneof quote_for {
+    uint64 amount_to_sell_in_sats = 1;


one might want to "sell $100 worth of bitcoin", so the amount as an input should be dollar, not sats.

That’s what the next line is for. You can specify either the btc amount to sell or the USD amount to buy (your scenario). See the oneof keyword

it's a different use case. yours is buy BTC with a USD amount. the one I mention is sell BTC with a USD amount

There is a matrix of 2x2x2 = 8 use cases.

The 3 binary variables are:
buy / sell USD
denominate amount in BTC / USB
execute immediately / execute in future

The api of the current price server has each of the 8 use cases as its own function call.
In this api I'm proposing to reduce the number of exposed endpoint to 2 (buy / sell) and pass the other binaries as function variables (denomination / execution).

If as a user, I request to sell $100 of bitcoin, but get credited 99.98 because of some math approximation and because if a mismatch of source of truth, then I would think there are hidden fees in here or that the system doesn't really work well.

If stablesats is going to be the source of truth, we can make those shortcuts because it would worsen the user experience and reduce trust in the system

So in your example the request would call GetQuoteToBuyCents(GetQuoteToBuyCentsRequest) because we are buying USD (ie. selling bitcoin). But we want to denominate the amount in usd ($100 worth). That means in the oneof argument I would specify the amount_to_buy_in_cents = 100

message GetQuoteToBuyCentsRequest { oneof quote_for { uint64 amount_to_sell_in_sats = 1; uint64 amount_to_buy_in_cents = 2; } bool immediate_execution = 3; } => GetQUoteToBuyCentsRequest { quote_for: { amount_to_buy_in_cents: 100 } immediate_execution: false }

This is the same way it works now with the GetSatsFromCentsForFutureBuy call.

The only difference here is that we are passing some arguments instead of separating each method call.

ok, I misread the initial interface.

If I want to buy $100 of bitcoin, can I get a quote by passing a dollar amount?

Yes (see example above)

ok.

I think message GetQuoteToBuyCentsRequest

would be better worded withmessage GetQuoteToBuyDollarRequest or message GetQuoteToBuyStablesatsRequest

because otherwise it's a bit convoluted that we using cents as both the assets and unit of account.

This may be why I have been mis interpreting the API (I skimed through it and didnt' try to read it thoroughly)

nicolasburtey · 2023-09-13T15:19:18Z

Cargo.toml

@@ -4,6 +4,7 @@ members = [
  "shared",
  "ledger",
  "price-server",
+  "quotes-server",


does it make sense to be different than the price server?

Easier to immutably replace rather than try to merge legacy with new server IMO

vindard · 2023-09-15T13:16:34Z

proto/quotes/quote_service.proto

+  rpc AcceptQuote(AcceptQuoteRequest) returns (AcceptQuoteResponse) {}
+}
+
+message GetQuoteToBuyCentsRequest {


I know we don't support this as yet, but do we also want to be more specific about the currency we're dealing with here, or more generic about the unit somehow?

For e.g. if we decided to add Euros to stablesats in the future somehow, a quote could be for buying EurCents or UsdCents. There's also the possibility that we use a non-cent currency like GBP (penny) or JPY (no minor unit).

We can pivot on the currency codes instead and think about some way to genericize the minor unit we're using? E.g.:

GetQuoteToBuyUsdRequest with a base and an offset for the request params / response fields?

Not sure if this is premature optimization though?

Premature IMO.

vindard · 2023-09-15T13:37:04Z

Another general comment. I have the problem right now where it's not easy to reason about whether I'm using the right quote type for the right circumstance. I'd have to refer back to this test case to see how the different prices behave with spreads, and the implications for using the wrong method are that we open ourselves up to arbitrage attacks.

I like that with these changes we explicitly have only 2 options, BuyCents or SellCents, which suggests that with a buy the spread should always be in one direction and with a sell in the other direction. I'm wondering if there's a way to capture this spread behaviour more explictly in the new api still. Maybe we can do this by returning the spread applied (amount, and whether +'ve or -'ve) in the response as an option?

bodymindarts · 2023-10-05T11:33:36Z

migrations/20231004130543_stablesats_quotes.up.sql

+    direction direction_enum NOT NULL,
+    immediate_execution BOOLEAN NOT NULL,
+    sat_amount BIGINT NOT NULL,
+    cent_amount BIGINT NOT NULL,


Do these need to be indexed?

bodymindarts · 2023-10-05T11:34:23Z

quotes-server/src/quote/entity.rs

@@ -3,6 +3,7 @@ use derive_builder::Builder;
 use serde::{Deserialize, Serialize};

 use crate::currency::*;
+use crate::EntityEvents;


this shouldn't be exposed in top level. Also use crate::{..}

bodymindarts · 2023-10-05T11:36:56Z

quotes-server/src/app/mod.rs

            .price_calculator
            .cents_from_sats_for_buy(Satoshis::from(sats), immediate_execution)
            .await?;
        let new_quote = NewQuote::builder()
            .direction(Direction::BuyCents)
            .immediate_execution(immediate_execution)
+            .cent_amount(usd_amount)
+            .sat_amount(Satoshis::from(sats))


redundant Satoshis::from.. just set it to a local var the same you use line 77.

bodymindarts · 2023-10-05T11:38:44Z

quotes-server/src/price/error.rs

@@ -2,10 +2,13 @@ use thiserror::Error;

 use shared::time::*;

+use crate::cache::OrderBookCacheError;


newline missing?

quotes-server/tests/quotes_app.rs

bodymindarts · 2023-10-26T19:50:18Z

quotes-server/tests/quotes_app.rs

+    let quote = app
+        .quote_cents_from_sats_for_buy(dec!(100_000_000), false)
+        .await;
+    tokio::time::sleep(std::time::Duration::from_secs(5)).await;


why not 2? And magic number...

bodymindarts · 2023-10-27T07:21:38Z

ledger/src/balances.rs

+            .await
+    }
+
+    pub async fn quotes_btc_liability(&self) -> Result<Option<AccountBalance>, LedgerError> {


btc is not a liability

bodymindarts · 2023-10-27T07:27:50Z

quotes-server/src/app/config.rs

+}
+
+fn default_expiration_interval() -> Duration {
+    const DEFAULT_EXPIRATION_INTERVAL: std::time::Duration = std::time::Duration::from_secs(120);


redundant const. default... fn already identifies unique access point + its not shared outside of this file or even outside this function.

bodymindarts · 2023-10-27T07:28:41Z

quotes-server/src/app/mod.rs

+            .expect("Could not build quote");
+        let quote = self.quotes.create(new_quote).await?;
+        if immediate_execution {
+            self.accept_quote(quote.id).await?;


Needs to be in transaction

bodymindarts · 2023-10-27T07:29:08Z

quotes-server/src/app/mod.rs

+            .direction(Direction::BuyCents)
+            .immediate_execution(immediate_execution)
+            .cent_amount(usd_amount)
+            .sat_amount(sats)


Missing cent_spread / sat_spread

bodymindarts · 2023-10-27T07:29:21Z

quotes-server/src/app/mod.rs

+            .expect("Could not build quote");
+        let quote = self.quotes.create(new_quote).await?;
+        if immediate_execution {
+            self.accept_quote(quote.id).await?;


needs to be in transactoi

bodymindarts · 2023-10-27T07:31:01Z

quotes-server/src/app/mod.rs

+            .expect("Could not build quote");
+        let quote = self.quotes.create(new_quote).await?;
+        if immediate_execution {
+            self.accept_quote(quote.id).await?;


accept_quote(id) ->
accept_quote_in_tx(quote, tx)

bodymindarts · 2023-10-27T07:31:40Z

quotes-server/src/app/mod.rs

+            return Err(QuotesAppError::QuoteExpired(id.to_string()));
+        }
+
+        quote.accept();


accept should fail with expired / already accepted errors.

bodymindarts · 2023-10-27T07:35:59Z

quotes-server/src/app/mod.rs

+                )
+                .await?;
+        }
+        self.quotes.update(quote).await?;


not atomic?

bodymindarts · 2023-10-27T07:37:55Z

shared/src/macros.rs

@@ -124,3 +124,58 @@ macro_rules! decimal_wrapper_common {
        }
    };
 }
+
+#[macro_export]
+macro_rules! entity_id {


sebastienverreault

spread calcs look good

thevaibhav-dixit · 2023-12-03T11:19:38Z

closing since most of the work on this has been done via multiple smaller pr's.

bodymindarts force-pushed the quotes branch from 66dd6de to 1623be6 Compare September 13, 2023 13:53

nicolasburtey reviewed Sep 13, 2023

View reviewed changes

vindard reviewed Sep 15, 2023

View reviewed changes

thevaibhav-dixit force-pushed the quotes branch from 8ce0459 to 282f89a Compare October 3, 2023 12:10

bodymindarts and others added 10 commits October 3, 2023 22:30

feat: quotes-server

0ff4862

chore: copy currency code to quotes

b6dc7ca

chore: more scoping under currency_exchange

fdf2eb1

refactor: rename currency_exchange -> price

cb680e8

chore: quotes app init

d1ff1a7

test: improve mixer test with Dummys

5f45af3

chore: price use case

5cbbd0c

test: all use cases of PriceCaclculator

c7ba7a6

refactor: rename proto api

8121681

feat: use order book in quotes server

990ea90

thevaibhav-dixit force-pushed the quotes branch from 282f89a to 990ea90 Compare October 3, 2023 19:22

thevaibhav-dixit added 2 commits October 4, 2023 16:10

chore: add entity folder for quotes-server

29e0bdb

feat: add entity events and repo

b95e5c3

thevaibhav-dixit force-pushed the quotes branch from a4aa81d to b95e5c3 Compare October 4, 2023 18:18

thevaibhav-dixit added 4 commits October 5, 2023 11:06

chore: quotes_server in cli

539a22b

chore: construct quote

3331a64

chore: run sqlx prepare

2f6d6db

feat: add sats/cents for sell/buy

e3b77c7

bodymindarts commented Oct 5, 2023

View reviewed changes

thevaibhav-dixit added 3 commits October 5, 2023 18:40

refactor: index only required fields

00ba01b

chore: address pr reviews

04cd534

chore: complete server_side code

76b52f7

test: add quotes_app test

edecb90

bodymindarts commented Oct 26, 2023

View reviewed changes

quotes-server/tests/quotes_app.rs Show resolved Hide resolved

thevaibhav-dixit added 3 commits October 27, 2023 01:09

refactor: add config based expiration_interval

b16e0e7

refactor: remove redundant errors

f45cf99

refactor: add accepted and expiration related errors

2b85777

bodymindarts commented Oct 26, 2023

View reviewed changes

test: cover various cases for quotes_app test

da7c5db

thevaibhav-dixit force-pushed the quotes branch from 0b7ca7b to da7c5db Compare October 26, 2023 20:05

bodymindarts commented Oct 27, 2023

View reviewed changes

quotes-server/src/app/mod.rs Outdated

)

.await?;

}

self.quotes.update(quote).await?;

Copy link

Member Author

bodymindarts Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not atomic?

bodymindarts commented Oct 27, 2023

View reviewed changes

refactor: checks on entity and make txn's atomic

7688148

thevaibhav-dixit force-pushed the quotes branch from 59f60ed to 7688148 Compare November 6, 2023 07:41

chore: add mark_events to entity_events

3dc60d9

thevaibhav-dixit force-pushed the quotes branch 3 times, most recently from 24c3f06 to 3dc60d9 Compare November 6, 2023 16:55

fix: use consistent style

5276163

thevaibhav-dixit force-pushed the quotes branch from c69dcb1 to e162af3 Compare November 11, 2023 18:48

feat: include cent/sat spread in quote(first-iteration)

bc940f4

thevaibhav-dixit force-pushed the quotes branch from e162af3 to bc940f4 Compare November 11, 2023 19:06

sebastienverreault approved these changes Nov 16, 2023

View reviewed changes

thevaibhav-dixit closed this Dec 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: quotes-server #430

feat: quotes-server #430

bodymindarts commented Sep 13, 2023

nicolasburtey Sep 13, 2023

bodymindarts Sep 13, 2023

nicolasburtey Sep 13, 2023 •

edited

Loading

bodymindarts Sep 13, 2023 •

edited

Loading

nicolasburtey Sep 14, 2023

bodymindarts Sep 14, 2023 •

edited

Loading

nicolasburtey Sep 15, 2023

bodymindarts Sep 15, 2023

nicolasburtey Sep 15, 2023

nicolasburtey Sep 13, 2023

bodymindarts Sep 13, 2023

vindard Sep 15, 2023

bodymindarts Sep 15, 2023

vindard commented Sep 15, 2023 •

edited

Loading

bodymindarts Oct 5, 2023

bodymindarts Oct 5, 2023

bodymindarts Oct 5, 2023

bodymindarts Oct 5, 2023

bodymindarts Oct 26, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

bodymindarts Oct 27, 2023

sebastienverreault left a comment

thevaibhav-dixit commented Dec 3, 2023

		@@ -2,10 +2,13 @@ use thiserror::Error;

		use shared::time::*;

		use crate::cache::OrderBookCacheError;

feat: quotes-server #430

feat: quotes-server #430

Conversation

bodymindarts commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasburtey Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

bodymindarts Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodymindarts Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindard commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienverreault left a comment

Choose a reason for hiding this comment

thevaibhav-dixit commented Dec 3, 2023

nicolasburtey Sep 13, 2023 •

edited

Loading

bodymindarts Sep 13, 2023 •

edited

Loading

bodymindarts Sep 14, 2023 •

edited

Loading

vindard commented Sep 15, 2023 •

edited

Loading