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(corda): support 5.1 via TS/HTTP (no JVM) #3241

Merged

Conversation

adrianbatuto
Copy link
Contributor

@adrianbatuto adrianbatuto commented May 1, 2024

Commit to be reviewed


feat(corda): support 5.1 via TS/HTTP (no JVM)

Primary Changes
----------------
1. Updated plugin-ledger-connector corda to support Corda 5
2. Created corda-v5-flow.test for testing the Corda 5 endpoints

Fixes #2978
Fixes #3293
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.

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

@adrianbatuto we will need another issue to update some code changes. Mentioning it here so that we don't loos the track of the same.

  1. Update each new endpoint function (in the connector class) to use one universal function to do GET/POST (Currently the universal function, setupRequest, does only the basic initialization)
  2. Update the AIO to not use --network host tag so that it can be tested via CI

@adrianbatuto adrianbatuto marked this pull request as ready for review May 9, 2024 07:27
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.

@adrianbatuto Two questions:

  1. Does this work without the JVM app? I see you've made changes to the kotlin code.
  2. Would merging this still retain compatibility with 4.x Corda ledgers?

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

Blocking the PR for now as the image used in this from @adrianbatuto personal github registry.
Once the AIO docker image gets published from the Cacti account itself, the process of merging of this PR can progress.

@adrianbatuto
Copy link
Contributor Author

@adrianbatuto Two questions:

  1. Does this work without the JVM app? I see you've made changes to the kotlin code.
  2. Would merging this still retain compatibility with 4.x Corda ledgers?
  1. Yes it is working without the JVM app. The changes to the kotlin code happened after running yarn run codegen.
  2. It should retain compatibility with Corda 4. I also ran some of the existing tests for Corda 4 to make sure.

@adrianbatuto
Copy link
Contributor Author

The corda-v5-flow.test.ts is getting flaky results so I added a timeout as a temporary fix. I will look further into the issue and a separate ticket might be created for it.

@adrianbatuto
Copy link
Contributor Author

adrianbatuto commented May 27, 2024

Blocking the PR for now as the image used in this from @adrianbatuto personal github registry. Once the AIO docker image gets published from the Cacti account itself, the process of merging of this PR can progress.

@jagpreetsinghsasan this has been resolved.

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.

@adrianbatuto - OK, thank you for confirming the answers and the PR.

@jagpreetsinghsasan Please make sure that the follow-up issues are opened and added to the board! Thank you in advance!

@jagpreetsinghsasan
Copy link
Contributor

@petermetz we have added the follow up tasks with the flakiness fix being the highest priority task in the queue followed by the cordapps feature issue.
This PR looks good to me now and can be merged once @adrianbatuto rebase it and fix conflicts and we have a merge consensus from other maintainers.

@jagpreetsinghsasan
Copy link
Contributor

@hyperledger/cacti-maintainers

@petermetz
Copy link
Contributor

@petermetz we have added the follow up tasks with the flakiness fix being the highest priority task in the queue followed by the cordapps feature issue. This PR looks good to me now and can be merged once @adrianbatuto rebase it and fix conflicts and we have a merge consensus from other maintainers.

@jagpreetsinghsasan Thank you! Agreed on all of those!

@adrianbatuto adrianbatuto force-pushed the adrianbatuto/issue2978 branch 4 times, most recently from 9014731 to ebdb9ee Compare July 23, 2024 03:44
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.

@adrianbatuto Sorry, I wasn't perfectly clear in one of my comments: https://github.com/hyperledger/cacti/pull/3241#discussion_r1712368658

@adrianbatuto adrianbatuto force-pushed the adrianbatuto/issue2978 branch 3 times, most recently from a0397ad to c960449 Compare August 12, 2024 06:37
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.

@adrianbatuto We are almost there!

@adrianbatuto adrianbatuto force-pushed the adrianbatuto/issue2978 branch 2 times, most recently from 3cd5b23 to 5e7ff7b Compare August 15, 2024 10:54
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.

@adrianbatuto Looking good! Now the last thing (hopefully) is to fix the failing corda connector server build

Cactus_CI / ghcr-connector-corda-server (pull_request) Failing after 1m
Details

@adrianbatuto
Copy link
Contributor Author

@adrianbatuto Looking good! Now the last thing (hopefully) is to fix the failing corda connector server build

Cactus_CI / ghcr-connector-corda-server (pull_request) Failing after 1m
Details

@petermetz, I fixed the build error and now the check is failing because of vulnerability issues.
image

Fixes hyperledger-cacti#2978
Fixes hyperledger-cacti#3293

Signed-off-by: adrianbatuto <adrian.batuto@accenture.com>
@petermetz
Copy link
Contributor

@adrianbatuto Thanks for the fix, we can address the trivy vulnerabilities later in a different PR, so, looking good to me now.

@petermetz petermetz merged commit ec9683d into hyperledger-cacti:main Aug 19, 2024
143 of 145 checks passed
@petermetz petermetz deleted the adrianbatuto/issue2978 branch August 19, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test(connector-corda): fix flaky corda-v5-flow.test.ts feat(corda): support 5.1 via TS/HTTP (no JVM)
4 participants