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

Settlement Engine Framework and Ethereum Ledger Settlement Engine #125

Merged
merged 8 commits into from Jul 28, 2019

Conversation

@gakonst
Copy link
Member

commented Jul 8, 2019

This PR defines a SettlementEngine trait and a tower-web service which exposes the endpoints from RFC536 and the forum thread. New engines must be implemented under the engines directory.

It also defines an Ethereum engine which makes settlements with onchain transactions, and notifies the connector once the onchain transaction has sufficient confirmations (adjustable param). You can run the engine by building and then:

./target/debug/interledger settlement-engine ethereum-ledger --key YOUR_ETH_PRIVATE_KEY --server_secret YOUR_REDIS_SECRET --confirmations NUM_CONFS

Notes:

@gakonst gakonst requested a review from emschwartz as a code owner Jul 8, 2019

@gakonst gakonst force-pushed the gakonst-eth-se branch 3 times, most recently from cc2d2d1 to 081c54b Jul 9, 2019

.circleci/config.yml Show resolved Hide resolved
configs/alice.yaml Outdated Show resolved Hide resolved
crates/interledger-settlement-engines/src/api.rs Outdated Show resolved Hide resolved
crates/interledger-settlement-engines/src/api.rs Outdated Show resolved Hide resolved
crates/interledger-store-redis/src/account.rs Outdated Show resolved Hide resolved
crates/interledger/src/node.rs Outdated Show resolved Hide resolved
scripts/configen.sh Outdated Show resolved Hide resolved
scripts/settlements_test.sh Outdated Show resolved Hide resolved
scripts/settlements_test.sh Outdated Show resolved Hide resolved

@gakonst gakonst force-pushed the gakonst-settlement-update branch from bc1649e to 160c59b Jul 13, 2019

@gakonst gakonst changed the base branch from gakonst-settlement-update to master Jul 13, 2019

@gakonst gakonst force-pushed the gakonst-eth-se branch 2 times, most recently from c9ae905 to 05b1b7c Jul 13, 2019

@gakonst gakonst force-pushed the gakonst-eth-se branch 4 times, most recently from e993a6d to 2480d23 Jul 15, 2019

@emschwartz
Copy link
Collaborator

left a comment

Lots of comments but great work on this so far!

configs/alice.yaml Outdated Show resolved Hide resolved
crates/interledger-api/src/routes/accounts.rs Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/configen.sh Outdated Show resolved Hide resolved

@gakonst gakonst force-pushed the gakonst-eth-se branch 5 times, most recently from 6b1e82c to a90d56b Jul 18, 2019

crates/interledger-api/src/routes/accounts.rs Outdated Show resolved Hide resolved
crates/interledger-settlement-engines/Cargo.toml Outdated Show resolved Hide resolved
.and_then(move |resp| {
parse_body_into_payment_details(resp).and_then(move |payment_details| {
store
.save_account_addresses(vec![account_id], vec![payment_details.to])

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 19, 2019

Collaborator

What happens if the Ethereum address is already linked to another account? Will the second registration overwrite the first? Will it fail? This seems like an important case to test

This comment has been minimized.

Copy link
@gakonst

gakonst Jul 19, 2019

Author Member

Currently it overwrites. We could make it fail if the key already exists, but I was thinking that this is a method some node operator might want to use to edit information of already stored accounts.

)
})
.and_then(
move |(_conn, addresses): (_, Vec<SlowHashMap<String, Vec<u8>>>)| {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 19, 2019

Collaborator

Sorry if I wasn't clear before. I think the problem is that the tuple you're trying to use doesn't match the schema of the data you're pulling out of Redis. Right now in Redis you have 4 fields under that key. If you try to set the type to a tuple with two fields, it will say the response was of incompatible type. In order to use a tuple with two fields, you would need to change what's being saved to Redis to remove the keys so there are only 2 values under that key.

This isn't a big issue -- it's a pretty minor difference whether we save the keys to the DB or not. I would say that if we had a lot more fields, we would definitely want to save the keys. If we only have 2 where one is required and the other is optional, I'd be fine leaving out the keys.

crates/interledger-api/src/routes/accounts.rs Outdated Show resolved Hide resolved
/// called when creating an account on the API
fn save_account_addresses(
&self,
account_ids: Vec<<Self::Account as Account>::AccountId>,

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 19, 2019

Collaborator

load_account_addresses takes a Vec<AccountId>, I thought that meant that each Address in the returned Vec would correspond to one of the AccountIds. Is that wrong? If so, why does it take the Vec<AccountId> in the first place?

fn send_money(
&self,
account_id: String,
money: Quantity,

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 19, 2019

Collaborator

Why does this need a special type if it's just wrapping a u64? If the API needs to pull it out of a JSON body, the API should be responsible for that and should just pass the u64 to this underlying method.

This comment has been minimized.

Copy link
@gakonst

gakonst Jul 19, 2019

Author Member

This is following the API from RFC536 https://github.com/interledger/rfcs/pull/536/files, I used the Quantity struct inside the function in case we decide to add the asset_scale field.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

Did we make a decision about this one way or the other?

@kincaidoneil
Copy link

left a comment

Nice job on this! Awesome to see the spec turning into code 💯

.settle_to(
addresses.own_address,
amount,
addresses.token_address,

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Jul 19, 2019

If the account says, "I'm using token X," but the SE either uses ETH, or is configured with token Y -- is there a check to prevent sending txs that will fail? (or ideally fail when creating an account)

This comment has been minimized.

Copy link
@gakonst

gakonst Jul 19, 2019

Author Member

A way to do this would be to have a local lookup of tickers to token addresses for this. The connector would then check if the received token_address from the peer matches the token address for the configured token ticker and only then it will return a success CREATED message to the user during account creation

})
.and_then(move |resp| {
parse_body_into_payment_details(resp).and_then(move |payment_details| {
let data = HashMap::from_iter(vec![(account_id, payment_details.to)]);

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Jul 19, 2019

Why isn't the payment tag actually saved/used?

To ensure incoming payments are credited to the correct account(s), I'm thinking the payment_details.tag should somehow be included as transaction data so the receiver can correlate it with the correct account (for ERC20 contract txs, could this be appended to the existing function call data without screwing anything up?)

If a peer says, "this is my ETH address," that should really only be interpreted as the address they want us to send payments to. Crediting incoming payments from their advertised address is insecure.

Alternatively, we could have them sign a message proving them control it, but I tend to prefer including a tag in each transaction because then multiple connector accounts can use the same ETH address, which is a nice feature

This comment has been minimized.

Copy link
@gakonst

gakonst Jul 19, 2019

Author Member

for ERC20 contract txs, could this be appended to the existing function call data without screwing anything up?

Nope. There is no way to do graffiti on transactions that involve smart contracts.

I prefer signing a message to prove ownership of a key, since we cannot meaningfully use the tag with ERC20 contracts. Does that mean we can remove the tag altogether? I did not get the need for it in the first place.

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Jul 19, 2019

There is no way to do graffiti on transactions that involve smart contracts.

Doesn't the EVM treat the appended data as an additional parameter to the function call? Why would this cause an issue? (e.g. isn't that how function overloading works in Solidity?)

I did not get the need for it in the first place.

The sender is explicitly telling the recipient which account to credit the money to. That's useful if:

  • Multiple accounts on the connector use the same ETH account (e.g. an operator has multiple connectors that are each peered with this connector, so multiple accounts, but uses the same SE or ETH account for all their connectors; also, using the same ETH account for multiple connector accounts proved very useful on testnet vs having to regen new accounts for each session in our testing)
  • A sender later decides to switch to using a different ETH account (without any changes to their connector account)

(And I believe this is simpler to securely implement...?)

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

@gakonst is there any way to include extra data in the transaction that the receiver would get in the notification or be able to look up from the original tx record?

@gakonst gakonst referenced this pull request Jul 19, 2019
12 of 14 tasks complete
@gakonst

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Regarding Ethereum engine specific comments I have opened a separate issue for them: #149

This PR has got pretty large so I suggest that we solve all the Ethereum engine specific improvements in a separate PR, since at its current state it works (but lacks robustness / can use optimizations)

@gakonst gakonst force-pushed the gakonst-eth-se branch 3 times, most recently from 2ba3633 to c5e5813 Jul 19, 2019

crates/interledger-settlement-engines/src/main.rs Outdated Show resolved Hide resolved
crates/interledger-settlement/src/api.rs Outdated Show resolved Hide resolved
"[{:?}] settlement engine service for listening to incoming settlements. Interval: {:?}",
address, interval,
);
std::thread::spawn(move || {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 22, 2019

Collaborator

I'm not sure it makes sense to spawn an OS thread to run another Tokio Executor. Isn't the point of Tokio that it manages different OS threads to handle different user-level tasks?

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

Still wondering about this. I think it should just use tokio::spawn instead of creating another tokio runtime

@emschwartz emschwartz referenced this pull request Jul 24, 2019
5 of 6 tasks complete
use serde::Serialize;
use serde_json::Value;
use std::str::FromStr;
use tokio_executor::spawn;
use tokio_retry::{strategy::FixedInterval, Retry};

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

Looks like backoff does have a recommended way of handling permanent errors: https://github.com/ihrwein/backoff#permanent-errors


#[post("/accounts/:account_id/settlement")]
/// Forwards the data to the API engine's `send_money` function.
/// Endpoint: POST /accounts/:id/settlement

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

nit: we might want to make this settlements in the spec

}
}))
} else {
// otherwise just make the call w/o any idempotency saves

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator
Suggested change
// otherwise just make the call w/o any idempotency saves
// otherwise just make the call without idempotency (note this is risky and not advisable)

/// Filters out transactions where the `from` and `to` fields match the provides
/// addreses.
pub fn transfer_logs(

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator
Suggested change
pub fn transfer_logs(
pub fn filter_transfer_logs(

or find_transfers_in_logs

})
}

// TODO: Extend this function to inspect the data field of a

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

What would it look for in the data field? Could you link to some details about that so someone who isn't as familiar with ETH transactions might be able to figure it out later?

use http::StatusCode;

pub mod redis_ethereum_ledger;
pub mod redis_store;

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator
Suggested change
pub mod redis_store;
pub mod redis_store_common;

or redis_store_shared

Box::new(
cmd("SETNX")
.arg(ethereum_transactions_key(tx_hash))
.arg(true)

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

What is this for?

"[{:?}] settlement engine service for listening to incoming settlements. Interval: {:?}",
address, interval,
);
std::thread::spawn(move || {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

Still wondering about this. I think it should just use tokio::spawn instead of creating another tokio runtime

.and_then(move |id| {
self_clone.notify_connector(
id.to_string(),
amount.low_u64(),

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator

Is there any reason why an ERC20 would have enough precision that this would be truncating the actual amount someone sent in the settlement?

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Jul 26, 2019

Yes, any ERC-20 that uses a scale of 18, which most do, with a settlement that's over 18 units of that token.

This comment has been minimized.

Copy link
@gakonst

gakonst Jul 27, 2019

Author Member

Hm yeah that can be a problem, if the asset_scale is set to 18. IIRC what we said with Evan in a call was that we'd compromise with Gwei minimum values.

// Get the block at `block_number`
self.web3
.eth()
.block(BlockNumber::Number(block_number as u64).into())

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jul 26, 2019

Collaborator
Suggested change
.block(BlockNumber::Number(block_number as u64).into())
.block(BlockNumber::Number(block_number).into())

Don't think that cast does anything

@b20160

This comment has been minimized.

Copy link

commented Jul 26, 2019

Will I be able to use this pr to test my settlement engine has long as I expose the correct endpoints?

@gakonst gakonst force-pushed the gakonst-eth-se branch from 23f03d2 to 96250ac Jul 28, 2019

@gakonst gakonst merged commit 6b6dec0 into master Jul 28, 2019

1 check passed

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

@gakonst gakonst deleted the gakonst-eth-se branch Jul 28, 2019

@emschwartz emschwartz referenced this pull request Jul 30, 2019
2 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.