-
Notifications
You must be signed in to change notification settings - Fork 20
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
[WIP] Rework ResponseSendFundsOp #215
Conversation
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.
cargo test
passes, but that does not necessarily mean everything works correctly. @realcr it would be nice if you glanced over these changes and merge/reject them.
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.
Seriously good work on this one, it was really quick!
I sent some comments, please check them out.
I started working on implementing your feedback. When creating I replaced that code to also use the new macro. It looks okay, but I'm not sure if introducing new global macro is good for readability (& complexity). I can revert 5fb773f and 2d65648 if you want (the latter was created because |
First I want to say that I appreciate the initiative (: . My opinion is against exporting the |
How about marking |
If we have something like I had another idea, tell me what you think: The functions EDIT: Formatting. EDIT: About the design of the routes checks: I just read the current version from the last commit on this PR. Although it seems to do the correct thing, it was difficult for me to read. This could be a matter of taste of course, but I fear someone less smart than you will have to think a lot to understand the code. Some examples for elements I believe are hard to read:
Most likely if I ever look again on this code a year from now, it will be difficult to recall what the I have some idea about how to make it simpler (At least to my taste). I give here some outline: /// Check if no element repeats twice in the array
fn is_unique(array: &[T]) -> bool {
// ....
}
pub fn is_valid_as_part(&self) -> bool {
// Make sure that the array is not too long: Not over MAX_ROUTE_LENGTH - 1
// check is_unique(...)
}
pub fn is_valid(&self) -> bool {
// If first element equals the last element, consider only `array[1:...]`. Otherwise consider the full array
// check is_valid_as_part(...)
} EDIT: Did you figure out the issue with the destination node getting route that is of length > 0? |
I like your idea about I don't think of myself as smart, especially when it comes to graphs, lists (routes), algorithms over them etc., so I appreciate your feedback a lot. I'm having some problems with reverting commits, so I'm going to force-push this PR. I'm attaching 5fb773f in case it got lost somehow. |
I was thinking about documentation of Currently: /// Find the index of a public key inside the route.
/// source is considered to be index 0.
/// dest is considered to be the last index. At first, I thought about replacing it with: /// Find the index of a public key inside the route or route fragment.
/// In full route, source is considered to be index 0.
/// dest is always considered to be the last index. Except that in case of a cycle, source can be the last index. Now I'm not sure what to do with these docs. I also suspect that may be a reason why a test may fail (if not now, then maybe in the future). |
Speaking of Running
If you want to look into it, maybe you know what key it is? |
@pzmarzly : I think I understand the problem. For example, if the route is This means that we should have this code: // Remove ourselves from the remaining route.
route_tail.public_keys.remove(0);
// Remove next node from the route:
route_tail.public_keys.remove(0); At the Note that after you will change this code you should stumble upon the next bug: thread 'handler::tests::pair_basic::test_handler_pair_basic' panicked at 'internal error: entered unreachable code', components/funder/src/handler/sender.rs:169:23 To fix this bug, you should fix assert_eq!(route![].is_valid_part(), true);
I agree, we can leave the code the way it is. But just for the sports, you should know that changing the functions you wrote to be generic is trivial. You should add a clause like
If the doc is confusing, we can begin by erasing it (: |
Speaking of generic Thanks for looking into the non-empty route issue, I will fix it now. |
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.
Some code expected route[0]
to be their own key, I tried to fix that. It turns out not to be that easy.
test_handler_pair_basic
works, but there are 3 tests that fail with InvalidResponseSignature
:
mutual_credit::tests::test_request_response_cancel_send_funds
mutual_credit::tests::test_request_response_collect_send_funds
tests::tests::test_funder_forward_payment
I think test_request_response_cancel_send_funds is the simplest of them all, so it could make a good starting point. But I have enough for today.
Those functions are legacy. We used to have a freeze guard, but we don't have it anymore. Try to comment out
I even recommend to have
You helped a lot. Making this feature work should give enhanced privacy, so it's pretty useful. |
On my PC tests pass, CI should be ready soon. TIL Github orders commits in PRs by their creation date, so even though the order is "Get cargo..." -> "Delete..." -> "Reword...", Github displays them here as they were before reordering. Now all that seems to be left is refactoring |
@pzmarzly : Thanks, great work on this PR! |
As discussed in #208
EDIT: Closes #208 and closes #220