cacti icon indicating copy to clipboard operation
cacti copied to clipboard

feat(satp): satp first implementation (#11)

Open outsidethecode opened this issue 1 year ago • 6 comments

  • 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


Pull Request Requirements

  • [ ] Rebased onto 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.
  • [ ] Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • [ ] Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • [ ] Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • [ ] Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners For rebasing and squashing, here's a must read guide for beginners.

outsidethecode avatar Jan 22 '24 13:01 outsidethecode

FYI:

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.

petermetz avatar Mar 07 '24 20:03 petermetz

@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

petermetz avatar Mar 07 '24 20:03 petermetz

@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.

VRamakrishna avatar Mar 08 '24 10:03 VRamakrishna

@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

outSH avatar Mar 11 '24 10:03 outSH

@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 avatar Apr 09 '24 13:04 RafaelAPB

@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?

VRamakrishna avatar Apr 10 '24 11:04 VRamakrishna

@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 avatar May 08 '24 06:05 sandeepnRES

@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. I think confusion is because of the build issue that also I fixed along with this extra commit, but that is part of the satp commit, so satp commit is stable and isn't broken.

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. For e.g., the fabric-driver build will fail if its dependencies like protos-js aren't built before building fabric-driver, and my change is just doing some cleanup in this case and providing a message to build protos-js first.

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.

sandeepnRES avatar May 09 '24 08:05 sandeepnRES

Looks good to me.

RafaelAPB avatar May 09 '24 16:05 RafaelAPB

@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. I think confusion is because of the build issue that also I fixed along with this extra commit, but that is part of the satp commit, so satp commit is stable and isn't broken.

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. For e.g., the fabric-driver build will fail if its dependencies like protos-js aren't built before building fabric-driver, and my change is just doing some cleanup in this case and providing a message to build protos-js first.

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.

@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.

petermetz avatar May 09 '24 19:05 petermetz

@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.

petermetz avatar May 10 '24 16:05 petermetz

@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 avatar May 10 '24 17:05 sandeepnRES

@sandeepnRES Ohhh, OK, fair enough! Good luck and let me know if I can help with anything!

petermetz avatar May 10 '24 18:05 petermetz