Improve Asset Scale conversions #192

gakonst · 2019-08-05T13:01:46Z

Implements the logic discussed in Settlement Engine Asset Scale #185.
Removes settlement_engine_asset_scale since it is being passed in by the engine per request
Improves the XRP/ETH test to take into account different decimals and exchange rate

Example:
Connector is making transactions for an account with account_scale 9 and ETH, so it denominates funds in Gwei.

When the connector wants to settle, it sends the amount and 9 to the engine.

The engine is configured to use asset_scale = 18. The engine will proceed to make an on-chain transaction for amount * 1e9.

The receiver's engine will pick that up, and send (1e9, 18) to the receiver's connector. The receiver's connector will then downscale 1e9 from 18 to whatever asset_scale it uses for that account.

crates/interledger-settlement-engines/Cargo.toml

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

crates/interledger-settlement-engines/tests/eth_xrp_interoperable.rs

crates/interledger-settlement-engines/tests/test_helpers.rs

crates/interledger-settlement/src/lib.rs

gakonst · 2019-08-06T11:02:56Z

Test fails with:

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/eth_xrp_interoperable-4b4be70b3ccaf056

running 1 test
Listening for incoming XRP payments and polling Redis for accounts that need to be settled
Listening for incoming XRP payments and polling Redis for accounts that need to be settled
[2019-08-06T11:01:01Z ERROR interledger_btp::client] Error connecting to WebSocket server for account: 1 Io(Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" })
[2019-08-06T11:01:02Z ERROR interledger::node] No route found for outgoing account 1
[2019-08-06T11:01:02Z ERROR interledger::node] No route found for outgoing account 1
[2019-08-06T11:01:02Z ERROR interledger_http::client] Error sending HTTP request: Error(Hyper(Error(Connect, Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" })), "http://localhost:3010/ilp")
[2019-08-06T11:01:02Z ERROR interledger_http::client] Error sending HTTP request: Error(Hyper(Error(Connect, Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" })), "http://localhost:3020/ilp")
[2019-08-06T11:01:02Z ERROR interledger_settlement::api] Couldn't load idempotent data
[2019-08-06T11:01:02Z ERROR interledger_ccp::server] Got route broadcast with a routing loop (path includes us): Route { prefix: b"example.bob", path: [b"example.bob", b"example.alice"], auth: [43, 50, 219, 108, 44, 10, 98, 53, 251, 19, 151, 232, 34, 94, 168, 94, 15, 14, 110, 140, 123, 18, 109, 0, 22, 204, 189, 224, 230, 103, 21, 30], props: [] }
[2019-08-06T11:01:02Z ERROR interledger_settlement::api] Couldn't load idempotent data
[2019-08-06T11:01:02Z ERROR interledger_ccp::server] Got route broadcast with a routing loop (path includes us): Route { prefix: b"example.bob", path: [b"example.bob", b"example.alice"], auth: [43, 50, 219, 108, 44, 10, 98, 53, 251, 19, 151, 232, 34, 94, 168, 94, 15, 14, 110, 140, 123, 18, 109, 0, 22, 204, 189, 224, 230, 103, 21, 30], props: [] }
[2019-08-06T11:01:02Z ERROR interledger::node] No route found for outgoing account 1
[2019-08-06T11:01:02Z ERROR interledger_stream::client] Send money stopped because of error: SendMoneyError("Packet was rejected with error: F02 No outgoing route for account: 1 (ILP address of the Prepare packet: Address(\"example.bob.charlie.HBlM1WTNXUm0RN9MGPDgFjofyKeSWcEwWMN2FEeagMA\"))")
[2019-08-06T11:01:02Z ERROR interledger_spsp::client] Error sending payment: SendMoneyError("Packet was rejected with error: F02 No outgoing route for account: 1 (ILP address of the Prepare packet: Address(\"example.bob.charlie.HBlM1WTNXUm0RN9MGPDgFjofyKeSWcEwWMN2FEeagMA\"))")
time,max_amount_in_flight,amount_fulfilled
[2019-08-06T11:01:02Z ERROR interledger_api::routes::spsp] Error sending SPSP payment: SendMoneyError(70000)
Error sending SPSP payment: Error(Status(500), "http://localhost:3010/pay")
test eth_xrp_interoperable ... FAILED

I am unable to reproduce this locally. Relevant: hyperium/hyper#1757

The scale conversion will be performed on the engine. We must send our asset scale to the engine so that it knows how to handle the number we sent it.

…y the engine

…ine scale If we have configured the engine with asset scale 18, and the connector sends a body with scale 9 and amount 1, we should settle for 1e9

For the connector: if the number after conversion would still not fit in a u64, we use the u64::MAX

instead of constants

Previously the Convert trait implementation had an arithmetic error, which was forcing us to use provide it parameters in the opposite order.

This reverts commit 36a3732.

…nstead of Error

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

crates/interledger-settlement-engines/tests/eth_xrp_interoperable.rs

crates/interledger-settlement/src/api.rs

emschwartz · 2019-08-07T12:45:54Z

crates/interledger-settlement/src/api.rs

+                        to: account.asset_scale(),
+                    });
+                    // If we'd overflow, settle for the maximum u64 value
+                    let safe_amount = if let Some(amount) = amount.to_u64() {


I think it's slightly less of a foot-gun to do this instead of using a different variable name:

Suggested change

let safe_amount = if let Some(amount) = amount.to_u64() {

let amount = if let Some(amount) = amount.to_u64() {

I'd rather be explicit about the variable name here being safely scaled and leave it as safe_amount. Why do you think it's a footgun?

I like using shadowing when doing these kind of transformations on numbers, just to make sure that I don't accidentally use the wrong one in something after applying the transformation (and to make sure that anyone working on that code later will avoid the same mistake).

OK I agree with you.

crates/interledger-settlement/src/api.rs

crates/interledger-settlement/src/lib.rs

… conversions

emschwartz

Overall, looks good. The only thing I'm not sure about is how the conversion functions handle overflows. It seems like it would be better to produce an error if the numbers overflow, though I could be convinced otherwise

crates/interledger-service-util/src/exchange_rates_service.rs

crates/interledger-settlement/src/lib.rs

- If exchange rate normalization fails return a reject packet - In all other places we use BigUint so the conversion should never fail, but we still handle the error graciously - prefer shadwing of variable name to avoid using the wrong variable - clarify comment about SettlementEngineDetails

…nge rate is applied

emschwartz · 2019-08-08T18:10:56Z

crates/interledger-service-util/src/exchange_rates_service.rs

+
+            match outgoing_amount {
+                Ok(outgoing_amount) => {
+                    // f64 which cannot fit in u64 gets cast as 0


Suggested change

// f64 which cannot fit in u64 gets cast as 0

// If an f64 is larger than the maximum value for a u64 but gets

// cast to a u64, it will end up being 0

emschwartz · 2019-08-08T18:11:35Z

crates/interledger-service-util/src/exchange_rates_service.rs

+            match outgoing_amount {
+                Ok(outgoing_amount) => {
+                    // f64 which cannot fit in u64 gets cast as 0
+                    if outgoing_amount != 0.0 && outgoing_amount as u64 == 0 {


Will this check really work? 🤔

Added a test which shows that it does

emschwartz · 2019-08-08T18:14:11Z

crates/interledger-service-util/src/exchange_rates_service.rs

+                        request.original_amount, request.from.asset_code(), request.from.asset_scale(), request.from.id(),
+                        outgoing_amount, request.to.asset_code(), request.to.asset_scale(), request.to.id());
+                }
+                Err(_) => {


When would this return an error versus turning into 0? This code is kind of confusing so it would definitely be helpful to put some comments describing what's going on

…l correctly

* fix(settlement-client): do not convert scale The scale conversion will be performed on the engine. We must send our asset scale to the engine so that it knows how to handle the number we sent it. * feat(settlement): Implement asset scale conversion for u256 * feat(settlement): Normalize the amount properly when being notified by the engine * fix(eth-se): Scale the amount to settle for based on the body and engine scale If we have configured the engine with asset scale 18, and the connector sends a body with scale 9 and amount 1, we should settle for 1e9 * test(eth-se): adjust test for proper scale conversion * test(eth-xrp) Set ETHXRP exchange rate * fix(crate): Remove settlement_engine_asset_scale from account * improvement(settlement/engines): use BigUInt to handle big numbers * fix(settlement): Convert asset scale properly Previously the Convert trait implementation had an arithmetic error, which was forcing us to use provide it parameters in the opposite order. * feat(settlement/engine): Return None when idempotent data not found instead of Error * fix(settlement): Make asset scale conversions overflow-safe - If exchange rate normalization fails return a reject packet - In all other places we use BigUint so the conversion should never fail, but we still handle the error graciously - prefer shadwing of variable name to avoid using the wrong variable - clarify comment about SettlementEngineDetails * improvement(exchange-rate): make the scale conversion after the exchange rate is applied * test(exchange-rate): Add tests which verify that rate conversions fail correctly

gakonst requested a review from emschwartz as a code owner August 5, 2019 13:01

gakonst force-pushed the gakonst-asset-scale branch from ac21692 to bad3ff0 Compare August 5, 2019 13:31

gakonst mentioned this pull request Aug 5, 2019

Add examples for multi-hop and cross-currency payments #182

Closed

2 tasks

emschwartz requested changes Aug 5, 2019

View reviewed changes

gakonst requested a review from emschwartz August 6, 2019 14:26

gakonst changed the base branch from xrp-eth-integration to master August 6, 2019 18:50

gakonst changed the base branch from master to xrp-eth-integration August 6, 2019 18:55

gakonst added 16 commits August 6, 2019 22:01

fix(settlement-client): do not convert scale

ba54167

The scale conversion will be performed on the engine. We must send our asset scale to the engine so that it knows how to handle the number we sent it.

feat(settlement): Implement asset scale conversion for u256

5e576ab

feat(settlement): Normalize the amount properly when being notified b…

a04832a

…y the engine

fix(eth-se): Scale the amount to settle for based on the body and eng…

4fc57a2

…ine scale If we have configured the engine with asset scale 18, and the connector sends a body with scale 9 and amount 1, we should settle for 1e9

improve tester

f02937d

test(eth-se): adjust test for proper scale conversion

9395f53

test(eth-xrp) Set ETHXRP exchange rate

0041bd7

test(eth-xrp): Switch back to BTP

c32f27c

fix(crate): Remove settlement_engine_asset_scale from account

5b1e2a5

improvement(settlement/engines): use BigUInt to handle big numbers

a5874a9

For the connector: if the number after conversion would still not fit in a u64, we use the u64::MAX

test(settlement-engines): use asset_scale local variables per test

61ecec5

instead of constants

test(engine-eth-xrp): elaborate on how results are produced

642a29b

fix(settlement): Convert asset scale properly

725b9f0

Previously the Convert trait implementation had an arithmetic error, which was forcing us to use provide it parameters in the opposite order.

fix comment

ca46d74

Revert "test(eth-xrp): Switch back to BTP"

56237a6

This reverts commit 36a3732.

feat(settlement/engine): Return None when idempotent data not found i…

1f6f9a4

…nstead of Error

gakonst force-pushed the gakonst-asset-scale branch from 2645416 to 1f6f9a4 Compare August 6, 2019 19:14

gakonst changed the base branch from xrp-eth-integration to master August 6, 2019 19:14

docs: fix readme in interoperability test

9de2c7f

emschwartz requested changes Aug 7, 2019

View reviewed changes

gakonst added 2 commits August 8, 2019 11:19

fix(settlement): Make u64 conversions overflow-safe, and warn for f64…

b5b62ec

… conversions

docs: fix review comments

1c3a182

emschwartz reviewed Aug 8, 2019

View reviewed changes

crates/interledger-service-util/src/exchange_rates_service.rs Outdated Show resolved Hide resolved

crates/interledger-settlement/src/lib.rs Outdated Show resolved Hide resolved

gakonst force-pushed the gakonst-asset-scale branch from aba4bce to 4927f5d Compare August 8, 2019 15:46

improvement(exchange-rate): make the scale conversion after the excha…

d227963

…nge rate is applied

gakonst force-pushed the gakonst-asset-scale branch from 4927f5d to d227963 Compare August 8, 2019 15:57

emschwartz approved these changes Aug 8, 2019

View reviewed changes

test(exchange-rate): Add tests which verify that rate conversions fai…

fa69cd6

…l correctly

gakonst merged commit 6cf0782 into master Aug 9, 2019

sappenin mentioned this pull request Aug 20, 2019

Confirm lua script functionality interledger4j/ilpv4-connector#268

Closed

emschwartz deleted the gakonst-asset-scale branch September 9, 2019 20:46

sappenin mentioned this pull request Oct 16, 2019

Create Postgres BalanceTracker interledger4j/ilpv4-connector#337

Closed

sappenin mentioned this pull request Sep 3, 2020

Improve Balance Tracker Logic interledger4j/ilpv4-connector#670

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Asset Scale conversions #192

Improve Asset Scale conversions #192

gakonst commented Aug 5, 2019 •

edited

Loading

gakonst commented Aug 6, 2019 •

edited

Loading

emschwartz Aug 7, 2019

gakonst Aug 8, 2019 •

edited

Loading

emschwartz Aug 8, 2019

gakonst Aug 8, 2019

emschwartz left a comment

emschwartz Aug 8, 2019

emschwartz Aug 8, 2019

gakonst Aug 9, 2019

emschwartz Aug 8, 2019

	let safe_amount = if let Some(amount) = amount.to_u64() {
	let amount = if let Some(amount) = amount.to_u64() {

	// f64 which cannot fit in u64 gets cast as 0
	// If an f64 is larger than the maximum value for a u64 but gets
	// cast to a u64, it will end up being 0

Improve Asset Scale conversions #192

Improve Asset Scale conversions #192

Conversation

gakonst commented Aug 5, 2019 • edited Loading

gakonst commented Aug 6, 2019 • edited Loading

emschwartz Aug 7, 2019

Choose a reason for hiding this comment

gakonst Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

emschwartz Aug 8, 2019

Choose a reason for hiding this comment

gakonst Aug 8, 2019

Choose a reason for hiding this comment

emschwartz left a comment

Choose a reason for hiding this comment

emschwartz Aug 8, 2019

Choose a reason for hiding this comment

emschwartz Aug 8, 2019

Choose a reason for hiding this comment

gakonst Aug 9, 2019

Choose a reason for hiding this comment

emschwartz Aug 8, 2019

Choose a reason for hiding this comment

gakonst commented Aug 5, 2019 •

edited

Loading

gakonst commented Aug 6, 2019 •

edited

Loading

gakonst Aug 8, 2019 •

edited

Loading