freedomlayer / offset

Offset payment engine
https://www.offsetcredit.org
Other
164 stars 20 forks source link

Atomic payments implementation #199

Closed realcr closed 5 years ago

realcr commented 5 years ago

Ported the codebase to the new atomic payments design (See issue https://github.com/freedomlayer/offst/issues/196). Main enhancements:

Integration tests are still turned off, as they need to be modified to match the new system's API.

realcr commented 5 years ago

@pzmarzly : This PR contains the main changes for the new design. I turned off the integration tests for now because I still need to fix them.

I can't yet merge this PR because it doesn't pass the clippy warnings test and rustfmt. I will probably won't have time to do this until the end of next week.

Can you fix the clippy issues on this branch and possibly open a new PR that we can merge in?

EDIT: Note that we currently run clippy like this (See .travis.yml)

cargo clippy -- -A clippy::needless_lifetimes -A clippy::identity_conversion

Because of some bugs in this version of clippy.

pzmarzly commented 5 years ago

Okay

pzmarzly commented 5 years ago

Kinda implemented on this branch but Travis seems to have problems (it didn't notice any new branch yet), so I will test tomorrow.

In async fn handle_app_message in components/app_server/src/server.rs the issue was the match { ... } block with too many arms. I tried refactoring that function in many ways, but the only 2 ways I found out to work were:

enum x { A, B, C, D };
// let's say we want `match` to have only 2 arms
match x {
    A | B => match x {
        A => { ... },
        B => { ... },
        _ => panic!("logic error"),
    },
    C | D => match x {
        C => { ... },
        D => { ... },
        _ => panic!("logic error"),
    },
}

In the end I just whitelisted that issue, but I think some of the code I wrote may stay - server.rs indeed had problem with readability, with lots of copy-pasted code. I reordered functions, so sadly Github side-by-side view is not that good for showing that. The important part is in async fn handle_app_message: before vs after.

realcr commented 5 years ago

Kinda implemented on this branch but Travis seems to have problems (it didn't notice any new branch yet), so I will test tomorrow.

Thanks for the quick work on this one! I think that travis doesn't recognize your branch because we only configured travis to work with pull requests. Feel free to open a new pull request for your branch, so we can see how it works with the CI. EDIT: I am pretty sure you have permissions to open new PRs, but if you don't please send me a message and I will fix it.

In the end I just whitelisted that issue

I agree with this decision. There are actually many possible messages, I don't think we should attempt to hide it.

I looked at the modification. You did some pretty good work with the refactoring of handle_app_message, I think that it looks much better now.

One thing I I'm not sure about is the x in those statements:

...
SetFriendRelays(x) => to_funder!(SetFriendRelays(x)),
...

Earlier it was the actual name of the contents, which I personally prefer. I know it makes the lines shorter, but now it is harder for me to think about the contents of the variants. What do you think?

pzmarzly commented 5 years ago

I decided against naming the variables, as the logic is implemented elsewhere anyway. Meanwhile, in these lines the most important part is to not let the variants mix. I thought it'd be easier to spot an error here:

Variant1(x) => to_funder!(Variant1(x)),
Variant2(x) => to_funder!(Variant1(x)),
Variant3(x) => to_funder!(Variant3(x)),

than here:

Variant1(some_long_variable_name) => to_funder!(Variant1(some_long_variable_name)),
Variant2(other_long_name_because_why_not) => to_funder!(Variant1(other_long_name_because_why_not)),
Variant3(short_name) => to_funder!(Variant3(short_name)),

Though that's just the reasoning behind my decision, and I'm indifferent about which way we use. If you feel it'd be useful to have variable names there, I will put them back.

Speaking of Travis, I can trigger builds by opening PR against pzmarzly/master, so it's okay. Meanwhile in freedomlayer/offst, I have push permission, so I can even push commit to this PR.

pzmarzly commented 5 years ago

Apparently, Clippy uses some kind of cache that doesn't play well with git, especially after rebase -i-ing. On my machine, it was returning no lints, but on CI one, it did. I managed to fix clippy on my machine now, but that means that I still have lints to fix.

realcr commented 5 years ago

Closed in favour of PR #203.