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

feat(connector-polkadot): add connector pkg, openapi specs, test suite #2877

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

AnmolBansalDEV
Copy link
Contributor

@AnmolBansalDEV AnmolBansalDEV commented Nov 11, 2023

It's the updated PR for Polkadot support for Cacti that incorporates the efforts of @RafaelAPB @CatarinaPedreira along with my work during the Hyperledger Mentorship under the mentorship of @jagpreetsinghsasan @suvajit-sarkar @petermetz

COMMITS TO REVIEW

feat(polkadot): creation of polkadot AIO image

Primary Changes
---------------
1. Updated docker image to include multi-stage build
2. Added healthcheck to the image
3. Added Supervisord and removed unneccessary start script
4. Updated the Readme with relevant commands

feat(polkadot): update substrate test tooling

Primary Changes
---------------
1. Added correct healthcheck for ledger container
2. Update the Substrate test ledger testcases

feat(polkadot): creation of polkadot connector openapi specs, test case and connector class

Primary Changes
---------------
1. Created openapi specs for get-prometheus-exporter-metrics, get-transaction-info,
   get-raw-transaction, sign-raw-transaction, run-transaction, deploy-contract-ink,
   and invoke-contract endpoints
2. Created relevant types for the request as well as response for the above endpoints
3. Added generated code for these endpoints
4. Created connector class with functions to interact with the polkadot ledger
5. Created webservices to interact with the endpoint and the relevant class method
6. Created unit and integration testcases in jest to test base functionality of the connector

Secondary Changes
-----------------
1. Added an ink! contract for running the testcases
2. Added the polkadot connector to ci workflow
3. Created substrate.md to docs-cactus
4. Added the polkadot connector to tsconfig.json

feat(polkadot): creation of readme and architecture reference diagrams

Primary Changes
---------------
1. Added README.md for the connector
2. Added Architecture diagrams for the plugin

Secondary Changes
-----------------
1. Added the docker image for the polkadot connector

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.

@jagpreetsinghsasan
Copy link
Contributor

@AnmolBansalDEV can you squash merge commits into the logical ones?

@jagpreetsinghsasan jagpreetsinghsasan changed the title feat: cacti polkadot connector feat(polkadot): cacti polkadot connector Nov 14, 2023
@jagpreetsinghsasan jagpreetsinghsasan changed the title feat(polkadot): cacti polkadot connector feat(polkadot): add new connector plugin Nov 14, 2023
@jagpreetsinghsasan
Copy link
Contributor

Overall, great work done @AnmolBansalDEV 🌻

Copy link
Contributor

@outSH outSH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you!

I've added some things to fix before merge.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV

  1. Exception throws and re-throws are done with the appropriate utilities which are either
  • createRuntimeErrorWithCause() from the file at packages/cactus-common/src/main/typescript/exception/create-runtime-error-with-cause.ts or
  • The http-errors library's function collection accessed via createHttpError . For HTTP REST API endpoint related error handling where your exceptions communicate with the web layer, have a look at the patterns established in this diff
    https://github.com/hyperledger/cacti/pull/2869

    In short, instead of this:
    // This is wrong - the HTTP response's status code will be
    // set to 500 but it should be 400 because it was user error
    // not a bug in the server's production code.
    throw new Error(
    `${fnTag} Cannot deploy contract without keychainId and the contractName`
      );
    Do this:
     if (!keychainHasContract) {
     // This is better, because the HTTP response will 
     // contain a status code of 400 instead of 500
     const errorMessage =
     	`${fnTag} Cannot create an instance of the contract instance because` +
     	`the contractName in the request does not exist on the keychain`;
     throw new createHttpError[400](errorMessage);
     }
  1. Ensure that your are not swallowing valuable stack traces / exception messages by incorrectly serializing inner exceptions such as shown below (which is only one of many creative ways people accidentally come up with to destroy crash information). The below code will print Something bad happened instead of something cool: [Object object]
    try {
    	await doSomethingCool();
    }
    catch (ex: unknown) {
    	const errorMessage = `Something bad happened instead of something cool: ${ex}`;
    	throw new RuntimeError(errorMessage);
    }
  2. Ensure Linear git history by squashing your commits into a single one and then rebasing onto upstream/main (after having ran a git fetch --all)
  3. Ensure that yarn run custom-checks is not failing with issues in the code that you are modifying/adding.
  4. Ensure that the code you have written has test coverage in general and also, more specifically that negative cases are verified by your tests as well. An example for the latter through test cases that send in incorrect input intentionally and then verify that the exception thrown accurately explains what went wrong. An example of this principlan implemented in action can be seen at the test case here: packages/cactus-cmd-api-server/src/test/typescript/integration/jwt-unprotected-endpoint-authz-ops-confirm.test.ts.
  5. Ensure that your yarn.lock file is up to date. If you haven't added/modified/removed any npm dependencies then this step is not necessary. Otherwise you can ensure an up to date yarn.lock file by running yarn intsall. It is important to NOT delete the lock file and then recreate it from scratch because this might trigger unintended auto-upgrades on all transitive dependencies that are set to auto-upgrade by the maintainers of the packages that we depend on.
  6. Ensure that you are not using container images that you've self-published when it comes to testing infrastructure. If you've made changes to a container image (usually by altering a Dockerfile) then you can ask a maintainer to publish those changes to you to the official GitHub Container Registry repository of the project (e.g. ghcr.io/hyperledger/cacti-some-image-that-you-are-working-on)
  7. Ensure that your commit message describes what is being changed and if there are multiple things then please try to break it down into multiple pull requests for easier review. Studies show that the larger the pull requests of a repository are on average, the worse code quality usually is.

@AnmolBansalDEV
Copy link
Contributor Author

Thank you @petermetz for your review, I've incorporated the changes and I request your re-review.

  1. Updated the Errors thrown and have used the create-errors lib
  2. Parsed the error traces appropriately inside of webservices like this one https://github.com/hyperledger/cacti/blob/a24d90c7a5a41e40772bb7ecef2acf66f87ad7f7/packages/cactus-plugin-ledger-connector-polkadot/src/main/typescript/web-services/sign-raw-transaction-endpoint.ts#L95-L98
  3. Cleaned git history by rebasing and having a progressive commit history
  4. yarn run custom-checks is not failing due to my changes
  5. testcases cover negative cases as well like here https://github.com/hyperledger/cacti/blob/a24d90c7a5a41e40772bb7ecef2acf66f87ad7f7/packages/cactus-plugin-ledger-connector-polkadot/src/test/typescript/integration/invoke-ink-contract.test.ts#L128-L143
  6. yarn.lock contains changes for polkadot connector packages only
  7. Please publish the updated AIO image for the Substrate network present here https://github.com/hyperledger/cacti/tree/a24d90c7a5a41e40772bb7ecef2acf66f87ad7f7/tools/docker/substrate-all-in-one
    Also Kindly publish this polkadot connector image as well https://github.com/hyperledger/cacti/blob/0a57c36f287f18c603eb4acfb68d40f845381b8f/packages/cactus-plugin-ledger-connector-polkadot/dockerfile
  8. I've tried to keep the git history streamlined, please suggest changes if you don't find the history fit

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV Please

  1. Update the yarn.lock file (I've just did a rebase locally and then yarn install made changes to the lock file so you need to do the same and then commit the lock file)
  2. Rename dockerfile to Dockerfile just to match the conventions.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV FYI: I left a few comments on one of the test cases but the same change requests also apply for all the other test cases as well

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV FYI: I left a few comments on one of the test cases but the same change requests also apply for all the other test cases as well

@AnmolBansalDEV
Copy link
Contributor Author

@petermetz
I've rebased the PR onto the main branch and incorporated the changes mentioned.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV

I published the current branch's state as this canary release that you can use for the container:
@hyperledger/[email protected]

For my last round of change requests please see the comments above.

In general this is looking good and we should be good to go once these minor changes are also addressed.

cc: @jagpreetsinghsasan

@petermetz petermetz enabled auto-merge (rebase) February 24, 2024 01:38
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV Well, one last push to fix the checks. Please rebase onto upstream main and fix the codegen action issues:

Run actions/[email protected]
Error: yarn codegen script produced version control side-effects: source files have been changed by it that are otherwise are under version control. This means (99% of the time) that you need to run the yarn codegen script locally and then include the changes it makes in your own commit when submitting your pull request.

auto-merge was automatically disabled February 25, 2024 07:02

Head branch was pushed to by a user without write access

@AnmolBansalDEV AnmolBansalDEV force-pushed the lfx-connector branch 2 times, most recently from e9241db to 5ae638f Compare February 25, 2024 07:08
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnmolBansalDEV I re-generated the yarn lock file and updated the commit message and the PR title. LGTM now

@petermetz petermetz changed the title feat(polkadot): add new connector plugin feat(connector-polkadot): add connector pkg, openapi specs, test suite Feb 27, 2024
@petermetz petermetz force-pushed the lfx-connector branch 2 times, most recently from e4a2cc9 to 4e84be4 Compare February 27, 2024 05:28
@petermetz
Copy link
Contributor

@AnmolBansalDEV Also fixed the linter errors and the spell checker issues.

@petermetz petermetz enabled auto-merge (rebase) February 27, 2024 05:28
@AnmolBansalDEV
Copy link
Contributor Author

@petermetz thank you so much for fixing the issues! Is it now ready to be merged?

@petermetz
Copy link
Contributor

petermetz commented Feb 27, 2024 via email

@petermetz
Copy link
Contributor

@AnmolBansalDEV I also had to fix the codegen script failing because of some updates to the way we do the openapi generator ignore file handling nowadays. This was the last required test that was failing as far as I could tell so it should actually get merged very soon.

I also updated the commit message to include the issue linking declarations.

@AnmolBansalDEV
Copy link
Contributor Author

@petermetz the codegen test is still failing, I don't understand what's causing this. Can you please point me out how to fix this? I tried running yarn codegen locally but it gets stuck I guess due to the limitations of my EC2 instance. Can you please look into it?

Primary Changes
---------------
1. Created openapi specs for get-prometheus-exporter-metrics, get-transaction-info,
   get-raw-transaction, sign-raw-transaction, run-transaction, deploy-contract-ink,
   and invoke-contract endpoints
2. Created relevant types for the request as well as response for the above endpoints
3. Added generated code for these endpoints
4. Created connector class with functions to interact with the polkadot ledger
5. Created webservices to interact with the endpoint and the relevant class method
6. Created unit and integration testcases in jest to test base functionality of the connector

Secondary Changes
-----------------
1. Added an ink! contract for running the testcases
2. Added the polkadot connector to ci workflow
3. Created substrate.md to docs-cactus
4. Added the polkadot connector to tsconfig.json

Signed-off-by: Anmol Bansal <[email protected]>

=======================================================================

feat(polkadot): creation of polkadot AIO image

Primary Changes
---------------
1. Updated docker image to include multi-stage build
2. Added healthcheck to the image
3. Added Supervisord and removed unneccessary start script
4. Updated the Readme with relevant commands

Signed-off-by: Anmol Bansal <[email protected]>

=======================================================================

feat(polkadot): update substrate test tooling

Primary Changes
---------------
1. Added correct healthcheck for ledger container
2. Update the Substrate test ledger testcases

Signed-off-by: Anmol Bansal <[email protected]>

=======================================================================

feat(polkadot): creation of readme and architecture reference diagrams

Primary Changes
---------------
1. Added README.md for the connector
2. Added Architecture diagrams for the plugin

Secondary Changes
-----------------
1. Added the docker image for the polkadot connector

Fixes hyperledger/cacti#726
Fixes hyperledger/cacti#727
Fixes hyperledger/cacti#627

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

Signed-off-by: Anmol Bansal <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz
Copy link
Contributor

@petermetz the codegen test is still failing, I don't understand what's causing this. Can you please point me out how to fix this? I tried running yarn codegen locally but it gets stuck I guess due to the limitations of my EC2 instance. Can you please look into it?

@AnmolBansalDEV Oops, I forgot to delete a local ignore file. Should be good now, let's see.

BTW: What are the hardware specs on the EC2 machine that you are using? I'd want to know especially the amount of RAM It has.

@petermetz petermetz dismissed jagpreetsinghsasan’s stale review February 27, 2024 21:31

All the requests seems to have been addressed, we are good to go!

@petermetz petermetz merged commit 6a476a0 into hyperledger-cacti:main Feb 27, 2024
118 of 147 checks passed
@petermetz
Copy link
Contributor

@AnmolBansalDEV Congrats! Wohoo!

@AnmolBansalDEV
Copy link
Contributor Author

@petermetz the codegen test is still failing, I don't understand what's causing this. Can you please point me out how to fix this? I tried running yarn codegen locally but it gets stuck I guess due to the limitations of my EC2 instance. Can you please look into it?

@AnmolBansalDEV Oops, I forgot to delete a local ignore file. Should be good now, let's see.

BTW: What are the hardware specs on the EC2 machine that you are using? I'd want to know especially the amount of RAM It has.

oh okay, I think the EC2 instance have 16GB RAM

@AnmolBansalDEV
Copy link
Contributor Author

@petermetz Thank you so much, it feels like achieving a major milestone!
Couldn't have been possible without the continuous support from @jagpreetsinghsasan

Also thanks to @outSH for valuable code reviews, and to @RafaelAPB and @CatarinaPedreira for foundational work!

@RafaelAPB
Copy link
Contributor

@AnmolBansalDEV it is a major milestone :D Congratulations for a great connector and thanks for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants