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

Generalize Account Ids #220

Merged
merged 25 commits into from Aug 19, 2019

Conversation

@gakonst
Copy link
Member

commented Aug 16, 2019

Part of fixing #186.

Adds a AccountId struct which uses Uuid internally for connector accounts. Each new generated account creates a new random uuid.

In order to send an SPSP payment to the non-default account, users are required to specify the receiver's uuid on the receiver's store. This is slightly bad UX, and will be solved by adding username aliases in a follow up PR.

The engine now no longer uses the Account trait (in preparation of the upcomign IldcpAccount into Account trait merge), and instead defines a id method in EthereumAccount.

gakonst added 11 commits Aug 16, 2019
test(engines): add helper to send to an SPSP account id
note that this does not support usernames, meaning the sender must know the internal store account's uuid which seems to introduce bad UX
feat(eth-se): Make engines use the AccountId datatype for account keys
Is this OK? This will use Uuid::parse_str under the hood, which fails if
the provided account id is not length 36 and or contains non-hex
characters
test(engines): Use explicit account UIDs for SPSP and checking balances
Previously we assumed that they were increasing, which is not the case now since each id gets randomly generated

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

@gakonst gakonst changed the title Generic Account Ids Generalize Account Ids Aug 16, 2019

@emschwartz
Copy link
Collaborator

left a comment

Overall, LGTM. Just a couple of minor comments

crates/interledger-ccp/src/lib.rs Show resolved Hide resolved
@@ -16,6 +15,7 @@ use log::{error, trace};

use crate::stores::redis_store_common::{EngineRedisStore, EngineRedisStoreBuilder};
use crate::stores::LeftoversStore;
use interledger_store_redis::AccountId;

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 16, 2019

Collaborator

I don't think this crate should depend on interledger_store_redis at all

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 16, 2019

Author Member

Running an engine requires an idempotent store.

I had copied it in the engines crate so that we did not depend on it, but then the idempotent store was refactored to be only in interledger_store_redis (which is what resulted in this PR #221) to avoid code duplication.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Aug 16, 2019

Collaborator

I don't understand. Isn't the IdempotentStore implemented in redis_store_common?

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 17, 2019

Author Member

I hadn't merged master at the time. Anyway, #221 is reverted in this PR, the redis dependency is only a dev dependency now for tests. We now use String types only in engine keys, in order to avoid making ANY assumption about what type of type the account_id is from the connector.

crates/interledger-store-redis/src/store.rs Outdated Show resolved Hide resolved
crates/interledger-store-redis/src/store.rs Outdated Show resolved Hide resolved
crates/interledger/src/main.rs Show resolved Hide resolved
crates/interledger/tests/test_helpers.rs Outdated Show resolved Hide resolved

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from e95a2af to a793311 Aug 16, 2019

gakonst added 4 commits Aug 17, 2019
implement IdempotentStore for the Ethereum store by forwarding to the…
… wrapped store

Rust does not support trait delegation
Remove Redis dependency from Engines
We need it as a dev-dependency when interacting to parse Accounts.
Engines now use raw-unparsed String datatypes as account ids. We remove the Copy trait and add clone's where necessary, but this allows us to do less type conversions

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from ffcfeea to fb511f6 Aug 17, 2019

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from fb511f6 to af1be75 Aug 17, 2019

gakonst added 3 commits Aug 17, 2019

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch 2 times, most recently from db64a56 to 7857851 Aug 17, 2019

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from 7857851 to 1f2bec4 Aug 17, 2019

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from 6a71c29 to 6d753f8 Aug 17, 2019

@gakonst

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

This keeps failing at random e2e tests with the error:

[2019-08-17T18:41:35Z 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")

Unclear what causes it. Note that it passes in the last run, but failed in a cargo-audit error regarding libcurl3 . I am unable to reproduce either issue locally.

We should pin the Rust version in Circle and update it explicitly.

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from ec5c54c to cc566d9 Aug 19, 2019

@gakonst gakonst force-pushed the gakonst-generic-account-ids branch from effd477 to 9b27f54 Aug 19, 2019

@gakonst gakonst merged commit 26d893d into master Aug 19, 2019

1 check passed

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

@emschwartz emschwartz deleted the gakonst-generic-account-ids branch Sep 9, 2019

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.