-
Notifications
You must be signed in to change notification settings - Fork 70
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
Get ILP Address from Parent node #276
Conversation
87ce8c6
to
47c5f4c
Compare
e601182
to
88c87e9
Compare
crates/interledger-settlement-engines/tests/eth_ledger_settlement.rs
Outdated
Show resolved
Hide resolved
81c2ffd
to
a6efec7
Compare
This started failing for no reason, the commit after which the CI started failing just modified a test. I cannot reproduce on my local machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments but overall this looks great!
.and_then(move |ilp_address| store.set_ilp_address(ilp_address)) | ||
.and_then(move |_| { | ||
// Get the parent's routes for us | ||
let prepare = RouteControlRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of constructing a RouteControlRequest
here, what do you think about having a handle to the CcpServer
to trigger it to send the request? If that's a lot more complicated, we can keep this as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but I'd prefer not. CcpRouteManager is generic over Incoming
, Outgoing
services and a Store
, and that'd require adding extra generic parameters on the API.
The 'benefit" would be being more explicit on the types of outgoing requests that we can make. I don't think that's really beneficial, since in the future we might want to perform other actions and send other types of requests, than just CCP requests via the outgoing handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good for now. Mind adding a TODO to make a note about this?
let prepare = RouteControlRequest { | |
// TODO we may want to make this trigger the CcpRouteManager to request | |
// routes instead of sending a RouteControlRequest here | |
let prepare = RouteControlRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor things but looks good to merge once they're addressed and the tests are passing
.and_then(move |ilp_address| store.set_ilp_address(ilp_address)) | ||
.and_then(move |_| { | ||
// Get the parent's routes for us | ||
let prepare = RouteControlRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good for now. Mind adding a TODO to make a note about this?
let prepare = RouteControlRequest { | |
// TODO we may want to make this trigger the CcpRouteManager to request | |
// routes instead of sending a RouteControlRequest here | |
let prepare = RouteControlRequest { |
.and_then(move |_| Ok(())) | ||
} | ||
|
||
fn connect_to_external_services<S, A, T, B>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very generic sounding name. It would be good to be more specific to what it's doing and/or put a comment explaining it
let service = BtpOutgoingService::new(ilp_address, next_outgoing); | ||
let mut connect_btp = Vec::new(); | ||
for account in accounts { | ||
// Can we make this take a reference to a service? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a lot of Arc
s and clones because futures 0.1 don't play nicely with references. We may be able to use a lot more references once we switch to futures 0.3 and async/await
.to_bytes(), | ||
); | ||
|
||
connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connection | |
// TODO check that the response is a success before proceeding | |
// (right now we just assume they'll close the connection if the auth didn't work) | |
connection |
let ilp_address = match details.ilp_address { | ||
Some(a) => a, | ||
None => { | ||
// if parent address ends with username, do not suffix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to have this. If you request your address using ILDCP, the parent will always append some segment that represents you. If you add a child account that you want to make your main receiving account, you want to differentiate the parent account on your own node (e.g. example.parent.your-username
) from the child account (e.g. example.parent.your-username.your-username
) to make sure packets go to the right account.
This will be used when inserting accounts which do not include an ILP Address. Their address will be our node's ilp address, followed by their username.
If an account is inserted without an , then 2 things can happen: 1. If their username is the same as the store's last segment, then their address is the same as the store. This allows to insert ourselves' account without having to specify our address 2. Otherwise, their address is created by appending their username to the store's ilp address Also adjust redis tests for new functionality. When the redis store is launched, it checks if the address is set and uses that, otherwise uses the default store address
This fixes the three_nodes test. Previously, the global_prefix would not get adjusted after our store had its ilp address updated. This resulted in _filter_routes_ and _update_best_routes_ to misbehave. By updating the ccp service's ilp address and global prefix before making any route broadcasts/receivals, we ensure that we always use the most up to date store address. Can this result in performance penalties, since we're performing an extra write? Probably not, since everything is done in memory
When a parent account is added, we must update our ILP Address in the store, so that the addresses of children accounts we add are calculated properly in the future. This is done via an ILDCP Request to the parent. We also need the parent's routes which we may care about, hence we send a RouteControlRequest as well.
Performed actions: 1. Connect to their BTP socket 2. Update our addresses if they were promoted to a parent 3. Connect to their settlement engine if one was added The helper function is now also called when updating an account's information.
fded56d
to
fe386fe
Compare
Closes #186.
Closes #248.
Closes #285
This PR modifies the address assignment procedure for nodes which get added as
Child
ren to a node.IldcpAccount
is merged withAccount
Child
, instead of using their provided ILP Address, we append theirusername
to our node's ilp address.Parent
(only 1 parent is allowed for now) the API:ILDCP
Request to Parent. The Parent responds with the ILP Address they have assigned the node, and the store's address is updated to that value. This is done so that any subsequent account insertions will take into account the node's updated ilp addresswhich has all the routes. This is done because when a node is started with a non-routable address, it rejects all incoming routing messages. This required to modify the CCP Service so that it's able to read the store's ILP Address on each broadcast loop, and update its
self.global_prefix
.Child
accounts on the node to use the update node's address. This can be an expensive operation, but we assume that a node operator will not change their parent node's address often.