-
Notifications
You must be signed in to change notification settings - Fork 286
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-iroha2): add support for Iroha V2 #2153
Conversation
@petermetz @izuru0 This is almost ready, I'm only waiting for fixes in upstream javascript SDK. Please start the review in the meantime |
...dger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
Fixed
Show resolved
Hide resolved
...dger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
Fixed
Show resolved
Hide resolved
...dger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
Fixed
Show fixed
Hide fixed
26938d1
to
9330dca
Compare
@petermetz @izuru0 After fixes from Iroha team this is ready for merge. Please review, I'll introduce future upgrades in separate PRs |
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
@petermetz can you review thanks
9330dca
to
19f28b6
Compare
- Add new endpoint `generate-transaction`, to create unsigned transactions that can be signed on the client side. - Add a function to iroha2-connector package to help signing iroha transactions on the client (BLP) side. - Extend transact endpoint to accept signed transaction as an argument as well. - Add new test suite to check features implemented in this PR (i.e. signing on the client side). Relates to hyperledger-cacti#2077 Depends on hyperledger-cacti#2153 Signed-off-by: Michal Bajer <[email protected]>
- Add new endpoint `generate-transaction`, to create unsigned transactions that can be signed on the client side. - Add a function to iroha2-connector package to help signing iroha transactions on the client (BLP) side. - Extend transact endpoint to accept signed transaction as an argument as well. - Add new test suite to check features implemented in this PR (i.e. signing on the client side). Relates to hyperledger-cacti#2077 Depends on hyperledger-cacti#2153 Signed-off-by: Michal Bajer <[email protected]>
.../cactus-plugin-ledger-connector-iroha2/src/main/typescript/plugin-ledger-connector-iroha2.ts
Outdated
Show resolved
Hide resolved
.../cactus-plugin-ledger-connector-iroha2/src/main/typescript/plugin-ledger-connector-iroha2.ts
Outdated
Show resolved
Hide resolved
...ges/cactus-plugin-ledger-connector-iroha2/src/main/typescript/web-services/query-endpoint.ts
Outdated
Show resolved
Hide resolved
.../cactus-plugin-ledger-connector-iroha2/src/main/typescript/web-services/transact-endpoint.ts
Outdated
Show resolved
Hide resolved
...dger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
Fixed
Show resolved
Hide resolved
19f28b6
to
1247411
Compare
Suggested edit: diff --git a/packages/cactus-plugin-ledger-connector-iroha2/openapitools.json b/packages/cactus-plugin-ledger-connector-iroha2/openapitools.json
index 29f5d069..601ac1d6 100644
--- a/packages/cactus-plugin-ledger-connector-iroha2/openapitools.json
+++ b/packages/cactus-plugin-ledger-connector-iroha2/openapitools.json
@@ -2,6 +2,6 @@
"$schema": "node_modules/@openapitools/openapi-generator-cli/config.schema.json",
"spaces": 2,
"generator-cli": {
- "version": "5.2.0"
+ "version": "5.2.1"
}
}
|
Suggested edit: diff --git a/packages/cactus-plugin-ledger-connector-iroha2/package.json b/packages/cactus-plugin-ledger-connector-iroha2/package.json
index aab130a5..83e9a8d8 100644
--- a/packages/cactus-plugin-ledger-connector-iroha2/package.json
+++ b/packages/cactus-plugin-ledger-connector-iroha2/package.json
@@ -1,6 +1,6 @@
{
"name": "@hyperledger/cactus-plugin-ledger-connector-iroha2",
- "version": "1.0.0",
+ "version": "1.1.0",
"description": "Allows Cactus nodes to connect to an Iroha V2 ledger.",
"keywords": [
"Hyperledger",
|
Suggested edit: diff --git a/packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts b/packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha2-instructions-and-queries.test.ts
similarity index 99%
rename from packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
rename to packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha2-instructions-and-queries.test.ts
index befa9a8e..cbe51a48 100644
--- a/packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha-instructions-and-queries.test.ts
+++ b/packages/cactus-plugin-ledger-connector-iroha2/src/test/typescript/integration/iroha2-instructions-and-queries.test.ts
@@ -1,5 +1,5 @@
/**
- * Tests for executing Iroha instructions and queries through the cactus connector.
+ * Tests for executing Iroha 2 instructions and queries through the cactus connector.
*/
//////////////////////////////////
|
@outSH Update about the 1.1.0 release packages being issued: we no longer have permission to directly publish to npm ourselves so it has to go through the github actions which I have a PR open for, but we'll have to test it to see if it even works and because we don't have the npm token of our own, this can only be tested "in production" as in the PR of mine that is adding the auto-publish action has to be merged and then we need to re-tag the release and see if it triggers and if the publishing process succeed or not (and if it does not then the whole PR+review+merge all over again until it gets fixed). Kinda off-topic but I wanted to keep you in the loop about what's taking so long. |
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.
@outSH Please rebase onto upstream/main to disappear the parent PR (the one that this one used to depend on) from the commit log of this PR
This PR/issue depends on:
|
@outSH FYI the v1.1.2 package artifacts are now published onto npm and can be used. |
1247411
to
d8d6600
Compare
I've also merged all the follow-up PR for this connector to avoid juggling back and forth fixes and make it more manageable in single PR. All PR I've merged were already approved so I hope it won't be a problem. The PR list:
I'll close them if you agree to merge it as one. |
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.
I'll close them if you agree to merge it as one.
Works for me. Ideally each PR would be as small as possible as a self contained change by itself, but in this case I think it's fine to combine them in the interest of moving things along a bit faster (I know it doesn't look like it but I try to be cognizant of productivity and velocity with my change requests haha)
Please do a final review and let me know if there's anything more you'd like to improve.
I left a little essay about the .npmrc thing in the comments above, please take a look at that, but other than that it's LGTM
e38acaa
to
6bedf9f
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.
@outSH LGTM, thank you!
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.
@outSH Sorry, last one: the tests need to pass too. (I retried them just now and they failed twice in a row so I'm thinking it's probably not a flake either).
I can't configure GitHub (yet) to require the passing of the tests for the iroha2 connector (because I can only pick checks to be mandatory that are already on the main branch) so I'll set that up once this is merged and in the meantime I'll just prevent accidental merging with the change request thingy. TLDR: Please fix the tests and then pass it back.
9bc9a86
to
6c715bd
Compare
@petermetz Yeah, there was a bug in Iroha2 test ledger healthcheck, sorry for that. Should be all good now. Please upload |
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.
@outSH Got it, thanks for the fix! I built and pushed the new image just now as: ghcr.io/hyperledger/cactus-iroha2-all-in-one:2022-12-22-6c715bdb
please re-test with the new image and then pass it back for review!
6c715bd
to
9dd6c71
Compare
- Add new Iroha V2 cactus connector. - Add endpoints `transact` and `query`. Both endpoints support critical subset of instructions and queries supported by the upstream javascript iroha sdk. Transaction can be awaited or can return immediately. - Add new endpoint `generate-transaction`, to create unsigned transactions that can be signed on the client side. - Add a function to iroha2-connector package to help signing iroha transactions on the client (BLP) side. - One SocketIO endpoint can be used to monitor new blocks from the ledger. - Add new helper method for signing query payload on the client side. - New connector can be used through a verifier-client interface. - All added functions are tested in functional test suites and documented. - Add test for complex scenario that involves creating new account and asset, and then transfering assets between two accounts. - Add test for parsing retrieved block data to find specific transaction hashes. - Added execution of Iroha2 tests to the CI. Additional notes: - Iroha V2 javascript packages are not available on official npm yet, had to include `.npmrc` with private npm address. I'm not sure if there's ETA of delivering these through NPM, so it might be necessary to commit it after all. Closes hyperledger-cacti#2138 Depends on hyperledger-cacti#2140 Signed-off-by: Michal Bajer <[email protected]>
9dd6c71
to
db78969
Compare
Done |
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.
@outSH Cheers, LGTM!
transact
andquery
. Both endpoints supportcritical subset of instructions and queries supported by the upstream javascript iroha sdk.
Additional notes:
in README. PR is not merge-ready until this is fixed (the CI should fail now).
.npmrc
withprivate npm address. I'm not sure if there's ETA of delivering these through NPM, so it might be
necessary to commit it after all.
Closes #2138
Depends on #2140
Signed-off-by: Michal Bajer [email protected]