cacti icon indicating copy to clipboard operation
cacti copied to clipboard

feat(connector): remove keychain dependency

Open TheJagpreet opened this issue 2 years ago • 2 comments

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

  • [x] 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.
  • [x] 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.
  • [x] 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.

TheJagpreet avatar Dec 18 '23 07:12 TheJagpreet

@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?)

TheJagpreet avatar Apr 03 '24 06:04 TheJagpreet

@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 https://github.com/hyperledger/cacti/pull/2952#discussion_r1589824232 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 avatar May 04 '24 00:05 petermetz

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

TheJagpreet avatar May 09 '24 06:05 TheJagpreet

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

petermetz avatar May 19 '24 23:05 petermetz

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

TheJagpreet avatar May 22 '24 06:05 TheJagpreet

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!

petermetz avatar May 24 '24 18:05 petermetz