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): remove keychain dependency #2952

Conversation

jagpreetsinghsasan
Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan commented Dec 18, 2023

Commit to be reviewed


feat(connector): remove keychain dependency

Primary Changes
---------------
1. Updated besu, ethereum, quorum and xdai connectors
to remove hard dependencies on keychain

Changes required to incorporate 1)
2. Updated openapi.json file of the above mentioned connectors
   to include the new no-keychain endpoints
3. Generated code and updated related web-services for the same

Fixes #963
Fixes #1163

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

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

@jagpreetsinghsasan Mostly LGTM, I Just left a few comments in the besu connector's implementation and I have one more question: Is there an endpoint implementation for the new method included or is it just the connector method for now and you are planning on adding the REST endpoint later in another PR (which is also reasonable because the PR is already pretty big)

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-963 branch 2 times, most recently from 14394b8 to 49d62e3 Compare April 3, 2024 06:25
    Primary Changes
    ---------------
    1. Updated besu, ethereum, quorum and xdai connectors
    to remove hard dependencies on keychain

    Changes required to incorporate 1)
    2. Updated openapi.json file of the above mentioned connectors
       to include the new no-keychain endpoints
    3. Generated code and updated related web-services for the same

Fixes hyperledger-cacti#963

Signed-off-by: jagpreetsinghsasan <jagpreet.singh.sasan@accenture.com>
@jagpreetsinghsasan
Copy link
Contributor Author

@jagpreetsinghsasan Mostly LGTM, I Just left a few comments in the besu connector's implementation and I have one more question: Is there an endpoint implementation for the new method included or is it just the connector method for now and you are planning on adding the REST endpoint later in another PR (which is also reasonable because the PR is already pretty big)

@petermetz I have added the endpoint for deployContractNoKeychain (do we need to do it for all the endpoints or just showcasing it for one endpoint works?)

@petermetz
Copy link
Contributor

@jagpreetsinghsasan Mostly LGTM, I Just left a few comments in the besu connector's implementation and I have one more question: Is there an endpoint implementation for the new method included or is it just the connector method for now and you are planning on adding the REST endpoint later in another PR (which is also reasonable because the PR is already pretty big)

@petermetz I have added the endpoint for deployContractNoKeychain (do we need to do it for all the endpoints or just showcasing it for one endpoint works?)

@jagpreetsinghsasan Sorry for the slow response, this one is a toughie. Let's keep it as small as possible initially. It's hard to review the bigger diffs. So just a single endpoint in a single package. Let's not do it all together. Also, if @outSH agrees to my idea presented in #2952 (comment) then we can cut down the scope of this PR to just the besu connector and just the single endpoint. After all that is done, I'll do a deeper dive with a much more detailed review where there'll be a lot of nit-picking (apologies in advance) because this is a very important PR/change in the sense that I'll want to use this as THE example for everyone else in the future for them to use as a template/inspiration for similar changes where we decouple things.

@petermetz petermetz requested a review from outSH May 4, 2024 00:25
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.

^^ @jagpreetsinghsasan @outSH Please see both of my comments above.

@jagpreetsinghsasan
Copy link
Contributor Author

@jagpreetsinghsasan Mostly LGTM, I Just left a few comments in the besu connector's implementation and I have one more question: Is there an endpoint implementation for the new method included or is it just the connector method for now and you are planning on adding the REST endpoint later in another PR (which is also reasonable because the PR is already pretty big)

@petermetz I have added the endpoint for deployContractNoKeychain (do we need to do it for all the endpoints or just showcasing it for one endpoint works?)

@jagpreetsinghsasan Sorry for the slow response, this one is a toughie. Let's keep it as small as possible initially. It's hard to review the bigger diffs. So just a single endpoint in a single package. Let's not do it all together. Also, if @outSH agrees to my idea presented in #2952 (comment) then we can cut down the scope of this PR to just the besu connector and just the single endpoint. After all that is done, I'll do a deeper dive with a much more detailed review where there'll be a lot of nit-picking (apologies in advance) because this is a very important PR/change in the sense that I'll want to use this as THE example for everyone else in the future for them to use as a template/inspiration for similar changes where we decouple things.

Sure @petermetz . I will then reduce the scope of this to just the Besu connector. Will close this PR once the conversation is concluded from all the involved maintainers.

@petermetz
Copy link
Contributor

@jagpreetsinghsasan Mostly LGTM, I Just left a few comments in the besu connector's implementation and I have one more question: Is there an endpoint implementation for the new method included or is it just the connector method for now and you are planning on adding the REST endpoint later in another PR (which is also reasonable because the PR is already pretty big)

@petermetz I have added the endpoint for deployContractNoKeychain (do we need to do it for all the endpoints or just showcasing it for one endpoint works?)

@jagpreetsinghsasan Sorry for the slow response, this one is a toughie. Let's keep it as small as possible initially. It's hard to review the bigger diffs. So just a single endpoint in a single package. Let's not do it all together. Also, if @outSH agrees to my idea presented in #2952 (comment) then we can cut down the scope of this PR to just the besu connector and just the single endpoint. After all that is done, I'll do a deeper dive with a much more detailed review where there'll be a lot of nit-picking (apologies in advance) because this is a very important PR/change in the sense that I'll want to use this as THE example for everyone else in the future for them to use as a template/inspiration for similar changes where we decouple things.

Sure @petermetz . I will then reduce the scope of this to just the Besu connector. Will close this PR once the conversation is concluded from all the involved maintainers.

@jagpreetsinghsasan OK, just please make sure to make a comment on that new PR with the relevant comment threads directly linked (so not just a link to the PR but also the direct links to the comment threads - I'm asking for this because the way the GitHub UI collapses the comment threads sometimes it's not easy to find later on where the conversation was actually happening)

@jagpreetsinghsasan
Copy link
Contributor Author

Sure @petermetz . I will have the right references to the conversations happened here in the new PR.
Closing this PR

@petermetz
Copy link
Contributor

Sure @petermetz . I will have the right references to the conversations happened here in the new PR. Closing this PR

@jagpreetsinghsasan Got it, thank you!

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.

feat(connector-besu): remove hard dependency on keychain feat(connector): remove hard dependency on keychain
3 participants