-
Notifications
You must be signed in to change notification settings - Fork 283
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
feat(satp): sample implementation of SATP standard using relays #3001
feat(satp): sample implementation of SATP standard using relays #3001
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outsidethecode I have a few nits (see above in the comments) but otherwise LGTM for the big picture. Please make sure to have @sandeepnRES or @VRamakrishna approve the PR because they are the primary maintainers for the weaver related code components.
packages/cactus-plugin-ledger-connector-polkadot/src/test/rust/ink/flipper/Cargo.lock
Outdated
Show resolved
Hide resolved
ec2f2d7
to
982cb09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your work and this contribution! I've added some comments, please note that some of them applies to entire commit (I didn't want to repeat same message in multiple places), so please review and apply the changes accordingly :)
weaver/sdks/fabric/interoperation-node-sdk/src/SatpAssetManager.ts
Outdated
Show resolved
Hide resolved
weaver/sdks/fabric/interoperation-node-sdk/src/SatpAssetManager.ts
Outdated
Show resolved
Hide resolved
FYI: Peter's changes:
|
@RafaelAPB @outsidethecode Also I rebased onto upstream/main and then force pushed so please make sure not to overwrite the changes if you'll be doing any code adjustments of your own |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @outSH's change requests were addressed.
@outsidethecode Left several suggestions for you in response to comments made by @outSH . Please fix these, and then, assuming the tests pass, we should be ready to merge. |
@outsidethecode @VRamakrishna I've added one more comment in previous review items, please have a look :) Also please remember to re-request the review so I get a signal to review it sooner |
@outsidethecode any updates on this? We would like to merge your contributions asap. Can you please solve conflicts, rebase, address the feedback, and push? |
@RafaelAPB There's one outstanding comment from Michal that I didn't notice earlier, but have just asked Zakwan to fix. @petermetz I think all your queries have been resolved. There's one item from Michal that's pending. I wonder why the tests aren't running though? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outsidethecode LGTM, thank you very much! Remember to squash the commits and fix the conflicts before merge :)
weaver/sdks/fabric/interoperation-node-sdk/src/SatpAssetManager.ts
Outdated
Show resolved
Hide resolved
a629927
to
a8ed48c
Compare
@petermetz I've fixed the build issue, along with some other issues that I found during this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petermetz I've fixed the build issue, along with some other issues that I found during this.
I have a separate commit for the makefile fix, because I want to keep the commit separate, and not combine in the large commit, so that in future if it needs to be reverted it can be done easily. I can actually create separate PR, but it seemed logical to add it in this PR.
@sandeepnRES The guiding principle we have for commits on the main branch is that any commit on the main branch should compile successfully. Splitting the commit the way you are doing it now goes in the opposite direction by intentionally leaving a commit in on main that we know has the build broken. If you'd like to ensure that the commit can easily be reverted I would save the diff of that commit as a text file and attach it to the pull request (and/or save it anywhere else you know you'll find it) an the squash the commits together to satisfy the no build breaks on main principle.
Sometimes the rules can be broken too (as it have happened in the past) if there's some good reason to but the saving the diff to a text file is a one liner bash command so I'm thinking this should be OK as a workaround. With all that said, if there is a reason still after this to keep the commits separate then let us talk more about it.
Hi @petermetz That build or the makefile isn't broken, in the sense the build works fine with the one big satp commit. In the separate commit about makefile, I just added a cleanup and a verbose message for the case where build fails, to tell that why the build has failed. This seemed to be unrelated to the big satp commit, that's why wanted to keep it separate. If you are okay, I can create a separate PR for it too, and remove the commit from this PR. Only reason I put it in this PR because it was a very small change related to components that are modified in this PR. |
Looks good to me. |
@sandeepnRES If the build wasn't broken then nevermind, I'm OK with it for the same reasons you explained. I just thought the build was not working at all. |
@sandeepnRES Now it's got seven commits though and the diff is much bigger as well. Maybe if it's this much I'd recommend starting a new PR for the house-cleaning/house-keeping changes separately if you can afford it time-wise. With that said, I'm also very strapped for time and so don't want to put up barriers that slow people down so this is strictly just an idea/suggestion. |
Hi @petermetz no actually we are seeing some CI tests failing, so Rama and me are trying to fix them. Once all tests passes, we will squash the commits into only 2 commits as we discussed. |
@sandeepnRES Ohhh, OK, fair enough! Good luck and let me know if I can help with anything! |
c81b1d6
to
ddb072f
Compare
3d0d730
to
7ec6f32
Compare
* Implemented the two endpoints TransferProposalClaims and TransferProposalReceipt * implemented the lock assertion method * Added the lock-assertion-receipt and lock-assertion-broadcast methods * Added the request to the driver to lock an asset * corrected some variable names * Added the endpoints corresponding to the steps 3.1 to 3.9 * Implemented the step 2.1B so that the driver call the gateway to update the asset status * Implemented the status check functionality required for 2.1B, 3.2B, 3.4B, and 3.6B * First iteration of switching to real driver (fabric) * Added the required functions to call back the gateway after the asset is locked. * Colorful log to show different stages and steps * Initial commit for create-asset, extinguish, and assign-asset * Added the resources required to assign an asset * uncommented the perform lock part * implemented the lock assertion method * Added the lock-assertion-receipt and lock-assertion-broadcast methods * Added the request to the driver to lock an asset * corrected some variable names * Added the endpoints corresponding to the steps 3.1 to 3.9 * Implemented the step 2.1B so that the driver call the gateway to update the asset status * Implemented the status check functionality required for 2.1B, 3.2B, 3.4B, and 3.6B * First iteration of switching to real driver (fabric) * Added the required functions to call back the gateway after the asset is locked. * Initial commit for create-asset, extinguish, and assign-asset * Added the resources required to assign an asset * uncommented the perform lock part * corrected compile error due to merging issue * Squashed commit: first implementation version of satp * implemented the lock assertion method * Added the lock-assertion-receipt and lock-assertion-broadcast methods * Added the request to the driver to lock an asset * corrected some variable names * Added the endpoints corresponding to the steps 3.1 to 3.9 * Implemented the step 2.1B so that the driver call the gateway to update the asset status * Implemented the status check functionality required for 2.1B, 3.2B, 3.4B, and 3.6B * First iteration of switching to real driver (fabric) * Added the required functions to call back the gateway after the asset is locked. * Initial commit for create-asset, extinguish, and assign-asset * Added the resources required to assign an asset * uncommented the perform lock part * corrected compile error due to merging issue * Initial documentation on how to run the satp gateway and test it * feat(weaver): add application logs * Initial log functionality * feat: log statements are stored in sqlite db * feat: updated the .gitignore file * feat: added log entries to all requests * feat: removed the unnecessary * feat: removed unnecessary comments * feat: added the missing methods in driver * feat: remove the unnecessary log statements * feat: initial rfcs for satp * initial satp github action script * corrected the github actions * Corrected the protoc version to be 3.17.3 and rebuild * corrected the version of cacti_weaver_protos_rs and added the copyright statement * removed the code copied from fabric-cli * feat: initial rfcs for satp * initial satp github action script * corrected the github actions * Corrected the protoc version to be 3.17.3 and rebuild * corrected the version of cacti_weaver_protos_rs and added the copyright statement * removed the code copied from fabric-cli * refactored the code in satp.ts * corrected the satpsimpleasset smart contract * fix(relay-docker): change server to alpine image and add libsqlite * refactor: rename the service to sample_service due to hardcoded values --------- Peter's changes: 1. I've removed the Cargo.lock file from the diff because it seemed unrelated to the change at hand. 2. Applied some formatting (but not all because as I just realized we don't yet have the linter working for the ./weaver/ subdirectories) 3. Removed dead code/unused variables 4. Adjusted `var` to `let` 5. Not perfect by any means but I'll work on some more incremental upgrades later in follow-up pull requests. --------- Rama's changes * fix(weaver-common-rs): remove outdated Cargo.lock to ensure compilation * chore(weaver-common): upgraded build depenencies for Weaver Common Protobufs in Rust * fix(weaver-relay): upgraded relay Dockerfiles for SATP support Updated relay cargo dependencies. Updated instructions in docs. * fix(weaver): bug and linter fixes in relay, Fabric driver, and Fabric SDK Ensured that TLS endpoints in the relay module had the 'https' URL prefix. Removed unnecessary variable declarations. * chore(weaver-fabric-driver): removed unnecessary variables as flagged by Yarn Co-authored-by: Rafael Belchior <rafael.belchior@tecnico.ulisboa.pt> Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com> Co-authored-by: Sandeep Nishad <sandeep.nishad1@ibm.com> Co-authored-by: VRamakrishna <vramakr2@in.ibm.com> Signed-off-by: outsidethecode <49871473+outsidethecode@users.noreply.github.com> Signed-off-by: outsidethecode <outsidethecode@gmail.com> Signed-off-by: Rafael Belchior <rafael.belchior@tecnico.ulisboa.pt> Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: Sandeep Nishad <sandeep.nishad1@ibm.com> Signed-off-by: VRamakrishna <vramakr2@in.ibm.com>
…ries on build fail Signed-off-by: Sandeep Nishad <sandeep.nishad1@ibm.com>
7ec6f32
to
3fd54a4
Compare
This is a first-of-a-kind sample PoC implementation of the Secure Asset Transfer Protocol (SATP), which is currently being standardized in an IETF Working Group, using network-owned Cacti relays.
The network relays provided by the Weaver submodule are augmented with the gateway functions specified in the SATP Core draft. A pair of networks, each using a relay for cross-network communication, effect the secure transfer of an asset from one to the other in an atomic operation. The protocol diagram that represents this implementation is illustrated here. (Note: a new version of this diagram, v20, was published very recently, after this PR was submitted, though it has just cosmetic changes from v19.)
Note: This is purely a sample reference implementation of the SATP protocol that requires more work in future PRs to be generally usable. It only supports a specific asset transfer scenario across two basic Fabric test networks used in Cacti for CI/CD, and is meant only for demonstrative purposes. Though all SATP endpoints are implemented in the relays, the precise asset transfer scenario is hardcoded within the backend functions (both in the relay and in the Fabric driver, a.k.a. connector). This hardcoded logic will be removed very soon, making the relay correspond to a generic "gateway" appliance of the kind described in the SATP specifications.
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.