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

Merged all to one patch also added scripts for running locally #178

Closed
wants to merge 6 commits into from

Conversation

pr4u4t
Copy link

@pr4u4t pr4u4t commented Sep 13, 2022

#176 #175 #170
in addition scripts/local_run.sh for running 2pc locally for testing.

@HalosGhost
Copy link
Collaborator

Needs conflict-resolution, a clean rebase, and DCO.

@pr4u4t
Copy link
Author

pr4u4t commented Sep 13, 2022

I see working on it.

@pr4u4t
Copy link
Author

pr4u4t commented Sep 13, 2022

done

@HalosGhost
Copy link
Collaborator

I'll happily review shortly, but this has not had a clean rebase (the reason it's easy to tell is because there are merge commits). The easiest way for you to get to a clean state is probably git pull --rebase upstream trunk (assuming you have mit-dci/opencbdc-tx as a remote named upstream configured for you clone).

@pr4u4t
Copy link
Author

pr4u4t commented Sep 14, 2022

Done

@HalosGhost
Copy link
Collaborator

So, it makes perfect sense to me why ca23e90 and 5017223 should be in the same PR (really, they should be squashed to a single commit). But are all these changes otherwise related? On first blush, it looks like the separation you original had between the three (four) made more semantic sense:

  • Correct the snappy dependency issue
  • Add support for Arch to ./scripts/configure.sh
  • Move decode_address
  • (Add helper script for running 2pc locally)

Is there a reason to review these together?

@pr4u4t
Copy link
Author

pr4u4t commented Sep 14, 2022

Common goal of this changes is being able to develop and quick test REST daemon with client-cli functionality.

@HalosGhost
Copy link
Collaborator

These all may help toward that goal, but several of them do so only by-chance (e.g., the dependency issue affects anyone doing a build from a clean clone; adding support for Arch helps anyone using Arch; and the helper script may be helpful for everyone). Only one of these is specifically motivated (generally) by that use-case. I'd strongly recommend splitting these out more or less the way you had them before (much easier reviewer burden).

@HalosGhost
Copy link
Collaborator

I'm going to close this in favor of appropriately-separated PRs.

@HalosGhost HalosGhost closed this Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants