Closed jakinyele closed 3 years ago
Pushing fixes to address feedback shortly. Thanks, @yaymukund @marsella @DariusParvin !
Your changes look good to me. I will leave the others to resolve their convos.
Something weird seems to have happened with the commit history here. The diff is showing me all the changes made on main as well as your PR changes. I am not sure why, the commit hashes for all the main commits are the same. My guess is that the merge commit (ec6e3da) was done in an unusual way. Typically, I will pull the latest version of main and then, from my feature branch, run
$ git merge main
which will produce a merge commit message like
Merge branch 'main' into my-feature-branch
I don't know why the diff is so weird, typically it would hide identical changes. I think this will be resolved if you merge as described above right before you merge the PR into main, but please ping me when you're ready to do so. I want to double check that nothing sketchy happens to our history on main.
Your changes look good to me. I will leave the others to resolve their convos.
Something weird seems to have happened with the commit history here. The diff is showing me all the changes made on main as well as your PR changes. I am not sure why, the commit hashes for all the main commits are the same. My guess is that the merge commit (ec6e3da) was done in an unusual way. Typically, I will pull the latest version of main and then, from my feature branch, run
$ git merge main
which will produce a merge commit message like
Merge branch 'main' into my-feature-branch
I don't know why the diff is so weird, typically it would hide identical changes. I think this will be resolved if you merge as described above right before you merge the PR into main, but please ping me when you're ready to do so. I want to double check that nothing sketchy happens to our history on main.
Thanks! Yeah I noticed the same weirdness in the commit history but I think it was because I tried to rebase the branch on top of main and there were some conflicts. So, aborted and tried to merge main. I’ll see about redoing it to clean up the history
(After talking to Kenny in slack) I think with the way we launch the merchant and customer daemons in python means that even after you close the program with ctrl c
, they'll still be running in the background. I'll have a look if there's a way we can fix it but I just wanted to make a note of it here.
Also, I think the process for launching the merchant daemon ends up blocking the customer daemon, so the customer daemon doesn't actually get launched.
I'm thinking maybe there should be a separate function called run_daemon
which handles shutting down of the daemon when the program exits. And I think the customer and merchant daemons need to be run concurrently.
I am confident i can fix the issues I mentioned above but it's probably easiest for me to work on it after this PR has been merged.
This PR builds off the sandbox instructions here
The following command will set up the sandbox config for the customer/merchant and start the merchant server:
$: python3 test-zeekoe.py setup --url "http://localhost:20000" -v
Run the scenario command to establish a channel, make several payments, and then cust close:
$: python3 test-zeekoe.py scenario --channel 1 --num-payments 5 -v
$: python3 test-zeekoe.py scenario --channel 2 --num-payments 7 -v
The test script will be expanded to include other basic scenarios including customer closing on old state, merchant initiated close via expiry, and customer/merchant claim after the self delay config fix.