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(quorum-connector): implement validator interface on go-quorum-connector #1933

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Mar 17, 2022

  • Add three new endpoints to quorum ledger connector achieve legacy verifier compatibility.
    • InvokeRawWeb3EthContractEndpoint can be used to form any call to deployed contract.
    • InvokeRawWeb3EthMethodEndpoint can be used to call any web3.eth function. Both are marked as low-level functions, should be used only when there's no designated endpoint for given functionality yet.
    • WatchBlocksV1Endpoint can be used to monitor new block headers / data from the ledger. Type of the output is determined from input option flag.
  • Extend QuorumApiClient to support Verifier interface, that is: block monitoring, and sending sync/async requests. Sending requests is marked as deprecated, because user can use direct REST calls from generated ApiClient, nevertheless this API was requested by one of the teams.
  • Added functional tests for two new, request based endpoints.
  • Moved verifier-besu integration test to besu-test package.
  • Added verifier-quorum integration test, it supplements direct endpoint tests and provides a reference for API usage.
  • Added support for QuorumApiClient in Verifier.

Closes: #1604
Signed-off-by: Michal Bajer [email protected]

Notes

  • I can't make CodeQL to cooperate, it reports dynamic call errors, even though I explicitly check if methods are defined on a contract / object (also assert they type is a function). Tried different methods to make the scanner notice it but did not succeed. Please double check and mark as false positive if possible.

Depends on #1928
Depends on #1926

@outSH
Copy link
Contributor Author

outSH commented Mar 17, 2022

@petermetz @izuru0 @takeutak Please review (only the last commit)

@outSH
Copy link
Contributor Author

outSH commented Mar 21, 2022

I've updated quorum ledger plugin readme as requested by @izuru0:

  • Added section on QuorumApiClient shortly describing it's capabilities.
  • Updated running container commands.

Running containers doesn't work though because of missing run-time-error package. I've added it to package.json, can't test it because of reasons described in #1897.

I've also changed dockerfile to use upstream ghcr.io/hyperledger/cactus-cmd-api-server:v1.0.0 instead of local cactus-api-server:latest

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

@outSH
Copy link
Contributor Author

outSH commented Mar 22, 2022

@petermetz Thanks for the detailed review, appreciate it :) I've added fixes in a separate commit for easier review fix: fixing code review issues, will squash it once the PR is ready to merge.

@outSH outSH requested a review from petermetz March 22, 2022 14:11
@petermetz
Copy link
Contributor

@petermetz Thanks for the detailed review, appreciate it :) I've added fixes in a separate commit for easier review fix: fixing code review issues, will squash it once the PR is ready to merge.

@outSH My pleasure and thank you for the separate commits, that does make it easier! It's LGTM now so please squash+rebase prior merging.
If you need any help with addressing CodeQL errors just let me know we can work together to figure it out.

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.

@outSH LGTM

@outSH
Copy link
Contributor Author

outSH commented Mar 29, 2022

@petermetz Thanks! :)

As for CodeQL, I've wrote an explanation in notes already that I don't think I can solve them :( I've seen on my branch that similar issue already occurs here and in some other modules, so I assume it was ignored there. The problem is most probably false positive at this point, since all execution paths are checked (please review the affected functions in the connector to be sure)

@github-actions github-actions bot removed the dependent label Apr 7, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

@outSH
Copy link
Contributor Author

outSH commented Apr 7, 2022

@petermetz OK, I've rebased and cleared the commit history. Can you review the CodeQL related message I wrote earlier and merge if possible?

@outSH outSH requested a review from petermetz April 11, 2022 12:03
…nnector

Add three new endpoints to quorum ledger connector achieve legacy
verifier compatibility. InvokeRawWeb3EthContractEndpoint can be used to
form any call to deployed contract. InvokeRawWeb3EthMethodEndpoint can
be used to call any web3.eth function. Both are marked as low-level
functions, should be used only when there's no designated endpoint for
given functionality yet. WatchBlocksV1Endpoint can be used to monitor
new block headers / data from the ledger. Type of the output is
determined from input option flag. Extend QuorumApiClient to support
Verifier interface, that is: block monitoring, and sending sync/async
requests. Sending requests is marked as deprecated, because user can use
direct REST calls from generated ApiClient, nevertheless this API was
requested by one of the teams. Added functional tests for two new,
request based endpoints. Moved verifier-besu integration test to
besu-test package. Added verifier-quorum integration test, it
supplements direct endpoint tests and provides a reference for API
usage. Added support for QuorumApiClient in Verifier.

Closes: hyperledger-cacti#1604
Signed-off-by: Michal Bajer <[email protected]>
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.

@outSH LGTM, thank you!

@petermetz petermetz merged commit 8d36bea into hyperledger-cacti:main Apr 13, 2022
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.

refactor(ledger-connector-quorum): add socket.io connection to cmd-api-server (and cmd-socket-server)
3 participants