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

Improve Asset Scale conversions #192

Merged
merged 22 commits into from Aug 9, 2019

Conversation

@gakonst
Copy link
Member

commented Aug 5, 2019

  • Implements the logic discussed in #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.

@gakonst gakonst requested a review from emschwartz as a code owner Aug 5, 2019

@gakonst gakonst force-pushed the gakonst-asset-scale branch from ac21692 to bad3ff0 Aug 5, 2019

@gakonst gakonst referenced this pull request Aug 5, 2019
0 of 2 tasks complete
@gakonst

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

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

@gakonst gakonst requested a review from emschwartz Aug 6, 2019

@gakonst gakonst changed the base branch from xrp-eth-integration to master Aug 6, 2019

@gakonst gakonst changed the base branch from master to xrp-eth-integration Aug 6, 2019

gakonst added some commits Aug 5, 2019

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.
fix(eth-se): Scale the amount to settle for based on the body and eng…
…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
improvement(settlement/engines): use BigUInt to handle big numbers
For the connector: if the number after conversion would still not fit in a u64, we use the u64::MAX
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.

@gakonst gakonst force-pushed the gakonst-asset-scale branch from 2645416 to 1f6f9a4 Aug 6, 2019

@gakonst gakonst changed the base branch from xrp-eth-integration to master Aug 6, 2019

crates/interledger-settlement/src/api.rs Outdated Show resolved Hide resolved
to: account.asset_scale(),
});
// If we'd overflow, settle for the maximum u64 value
let safe_amount = if let Some(amount) = amount.to_u64() {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 7, 2019

Collaborator

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() {

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 8, 2019

Author Member

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?

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 8, 2019

Collaborator

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).

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 8, 2019

Author Member

OK I agree with you.

crates/interledger-settlement/src/api.rs Outdated Show resolved Hide resolved
crates/interledger-settlement/src/lib.rs Show resolved Hide resolved
crates/interledger-settlement/src/lib.rs Outdated Show resolved Hide resolved
@emschwartz
Copy link
Collaborator

left a comment

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-settlement/src/lib.rs Outdated Show resolved Hide resolved
improvement(settlement): return Error on overflow during conversion
- 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

@gakonst gakonst force-pushed the gakonst-asset-scale branch from aba4bce to 4927f5d Aug 8, 2019

@gakonst gakonst force-pushed the gakonst-asset-scale branch from 4927f5d to d227963 Aug 8, 2019


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

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 8, 2019

Collaborator
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
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 {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 8, 2019

Collaborator

Will this check really work? 🤔

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 9, 2019

Author Member

Added a test which shows that it does

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(_) => {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 8, 2019

Collaborator

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

@gakonst gakonst merged commit 6cf0782 into master Aug 9, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

dora-gt added a commit to dora-gt/interledger-rs that referenced this pull request Aug 15, 2019

Improve Asset Scale conversions (interledger-rs#192)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.