search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
896 stars 198 forks source link

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

Closed pr4u4t closed 1 year ago

pr4u4t commented 2 years ago

176 #175 #170

in addition scripts/local_run.sh for running 2pc locally for testing.

HalosGhost commented 2 years ago

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

pr4u4t commented 2 years ago

I see working on it.

pr4u4t commented 2 years ago

done

HalosGhost commented 2 years ago

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 commented 2 years ago

Done

HalosGhost commented 2 years ago

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:

Is there a reason to review these together?

pr4u4t commented 2 years ago

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

HalosGhost commented 2 years ago

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 commented 1 year ago

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