feat(plugin-htlc-eth-besu): add private HTLCs and forge build & test

[RafaelAPB]

Primary Change:

Adds privacy-preserving HTLCs to the htlc-eth-besu package.

Secondary Change(s):

1. Applied the remappings in the foundry.toml file which is local to the
   package that's being changed. I figured out the parameters by reverse
   engineering the output of foundry config
2. Also removed the remappings.txt file because it is deprecated according
   to the foundry issue tracker on GitHub.
3. ci(htlc): add foundry setup and tests to the besu HTLC package tests
4. build(package.json): removed global nohoist, add localized remappings

Co-authored-by: Peter Somogyvari [email protected]

Signed-off-by: Peter Somogyvari [email protected]
Signed-off-by: Rafael Belchior [email protected]

[jagpreetsinghsasan]

I went through this PR. But I am not aware of what private HTLCs means. Can you please provide me some reference URL(s) where I read about it?

[RafaelAPB]

@jagpreetsinghsasan thanks for taking the time to review this PR. A private HTLC is explained in detail here: https://medium.com/@rafaelbelchior/dlt-interoperability-and-more-%EF%B8%8F-24-privacy-preserving-cross-chain-atomic-swaps-bonus-lets-ff99a90714de

[RafaelAPB]

I can maintain this package. @petermetz updated the CI and removed dependencies from git submodules as we discussed in the pair programming meeting.

[jagpreetsinghsasan]

LGTM, thankyou for the PR

[RafaelAPB]

@petermetz is this PR ready?

[petermetz]

@RafaelAPB Looks close now, I just want to make sure that the git submodules are contained only in that specific job in the CI that needs them (a good enough trade-off IMO), that way we are not pushing the additional complexity at every contributor just a tiny subset and even for them only for the testing (e.g. the project build remains functioning without the sub modules)

I'd do the following:

1. remove the git modules file from the root. If it has to be there during test execution then re-create it dynamically from within the CI job that is installing foundry-rs
2. Change the order of steps in the CI job so that the ci.sh execution comes first and all the custom things happen afterwards. This further reduces the chance of the git modules messing things up for the tests.
3. Audit what is actually necessary in the diff. For example the nohoist declarations in the root package.json can probably also be removed. Another example is the eslintignore changes.
4. Eliminate the usage of magic strings such as "Hashed Time Lock Contract v0.1"

[RafaelAPB]

@petermetz thanks for the feedback. Fixed all the points.

[petermetz]

@petermetz thanks for the feedback. Fixed all the points.

@RafaelAPB Thank you for the quick updates! What about the nohoist configuration in the root package.json file? Did those turn out to be necessary in the end? (e.g. if you remove those, does the build and/or tests start failing for some reason? If yes, that's okay but then we should document in the commit message the what/why/how
Something like: Added new nohoist global configuration because otherwise x script was failing with this exact error (paste in the actual failure logs with stack traces and everything else).

The reason I'm paranoid about global build changes because they usually have knock-on effects (unforeseen bugs or more subtle issues like build performance degradation or limiting what other configuration changes we can make) down the line for the entire project so that's why I'm trying hard to not have the global build changed unless absolutely necessary and if it is absolutely necessary then we need to leave a truckload of breadcrumbs to future generations who'll have to fix bugs with little to no context of what we were doing back in the distant past (e.g. now). :-)

[RafaelAPB]

@petermetz thanks for your comments and explanations, Peter. Nohoist is necessary because Forge solidity tests cannot import modules outside of the root project (so the dependencies must be within the project's node_modules). I'll add this to the readme.

[RafaelAPB]

@petermetz why is the CI failing in so many jobs?
The job specific to this PR passes (https://github.com/hyperledger/cacti/actions/runs/5328212830/jobs/9652794912?pr=2418) but doesn't seem that the CI configures Foundry or runs the tests. Any idea why?

[petermetz]

@RafaelAPB Works for me, as in LGTM now. The Last push/clean-up nitpicks of mine (sorry, as usual) are as follows:

1. Please check out my suggestion ( https://github.com/hyperledger/cacti/commit/573d171c1998f520323c0ef0afbf4d388c3226c2 ) about localizing the config changes needed (remappings can be specified locally for foundry and then it doesn't clutter up the root package.json file)
2. Please squash into a single commit and
3. Make sure that the one commit the PR ends up with specifies that you are talking about besu eth HTLCs because if someone sees feat: add private htlcs in the release changelog (which is where it will end up automatically when we issue the next release) then they will assume that (since the scope wasn't specified as a single package) the entire project (and therefore every single supported ledger) received private HTLCs in a single commit. I highly recommend something like feat(plugin-htlc-eth-besu): add private HTLCs and forge build & test. This is still not completely accurate because you did make changes in other packages, but at least it is only slightly inaccurate, e.g. people won't assuming that 40+ packages were updated when it was only 3.
4. Make sure that the PR title is synced up to what the commit message ends up being
5. Make sure that the commit message also contains the information about the nohoist (copy pasting from the readme is perfect, no need to spend time rephrasing) or if you decided to apply my suggestion then change both of these accordingly of course (I have verified that my suggestion works for foundry build and test commands but maybe there is something else that is broken that I'm not aware of that does work with the nohoist so just let me know.
6. Make sure that the PR description is synced to the final commit message as well

[petermetz]

@petermetz why is the CI failing in so many jobs?
The job specific to this PR passes (https://github.com/hyperledger/cacti/actions/runs/5328212830/jobs/9652794912?pr=2418) but doesn't seem that the CI configures Foundry or runs the tests. Any idea why?

@RafaelAPB That's because the public service we use to download the open API generator jar files is failing, so it has nothing to do with your diff, don't worry about it! (As in, we are working on fixing it somehow and if that's the only thing that fails then your PR should be good to go all else being equal)

[RafaelAPB]

Thanks @petermetz

[petermetz]

Thanks @petermetz

@RafaelAPB You are welcome! :-)

[RafaelAPB]

@petermetz anything else missing?

[petermetz]

@petermetz anything else missing?

@RafaelAPB 2 and 6 ;-)

[RafaelAPB]

@petermetz thought that would be hard to mess up, but apparently happens :) should be good now

[petermetz]

@RafaelAPB I cleaned up the commit message (there was about 20 lines with the sign-off) and synced it to the PR description. LGTM now, please do check if you are also happy with the way I formatted the commit message!

[RafaelAPB]

@petermetz looks much better, thank you!

[petermetz]

@RafaelAPB FYI: I made some changes to the failing withdraw test case (correct secret/hash lock) and the contract's .json artifact (now it has the contract name once again because I compiled it with the VSCode solidity extension)

[petermetz]

@RafaelAPB FYI: I also had to revert the changes in the public-api.ts beacuse they would've been a breaking change (all imports for the contract json file would've started failing)