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(besu): remove hard dependency on keychain #3338

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

jagpreetsinghsasan
Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan commented Jun 20, 2024

Commit to be reviewed

feat(besu): remove hard dependency on keychain

Primary Changes
---------------
1. Updated besu connector to remove hard
   dependency on keychain

Changes required to incorporate 1)
----------------------------------
2. Updated openapi.tpl.json to have non-keychain
   endpoints
3. Generated code and updated web-services for them
4. Updated transact( ) and deployContract( ) fx
5. Added deployContractNoKeychain( ) fx

Fixes #963

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 Thank you for reducing the scope of the PR to just the Besu connector. Now we can get down to the details properly.
First order of business is the code-reuse. Right now, in the diff, there's just one more method added to the connector and most of the code of it seems like copy-paste with some minor tweaks. Please investigate if it can be decomposed to smaller utility functions that do not depend on state . An example I added recently is here in these files on how to do code-reuse in this scenario:

  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-impl.ts
  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-http.ts
  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-grpc.ts
  • Also within the connector itself:
  public async getBlock(
    request: GetBlockV1Request,
  ): Promise<GetBlockV1Response> {
    const ctx = { logLevel: this.logLevel, web3: this.web3 };
    const getBlockV1Response = await getBlockV1Http(ctx, request);
    this.log.debug("getBlockV1Response=%o", getBlockV1Response);
    return getBlockV1Response;
  }

@jagpreetsinghsasan
Copy link
Contributor Author

@jagpreetsinghsasan Thank you for reducing the scope of the PR to just the Besu connector. Now we can get down to the details properly. First order of business is the code-reuse. Right now, in the diff, there's just one more method added to the connector and most of the code of it seems like copy-paste with some minor tweaks. Please investigate if it can be decomposed to smaller utility functions that do not depend on state . An example I added recently is here in these files on how to do code-reuse in this scenario:

  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-impl.ts
  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-http.ts
  • packages/cactus-plugin-ledger-connector-besu/src/main/typescript/impl/get-block-v1/get-block-v1-grpc.ts
  • Also within the connector itself:
  public async getBlock(
    request: GetBlockV1Request,
  ): Promise<GetBlockV1Response> {
    const ctx = { logLevel: this.logLevel, web3: this.web3 };
    const getBlockV1Response = await getBlockV1Http(ctx, request);
    this.log.debug("getBlockV1Response=%o", getBlockV1Response);
    return getBlockV1Response;
  }

This is a nice way to modularize the functions even further, and thus our main connector class looks cleaner. I am currently in the progress of refactoring the code to do the same. I will re-request for review once done and tested.

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-963c branch 2 times, most recently from bc34a4b to 23d2110 Compare July 11, 2024 05:14
@jagpreetsinghsasan jagpreetsinghsasan changed the title feat(connector): remove hard dependency on keychain feat(besu): remove hard dependency on keychain Jul 11, 2024
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-963c branch 2 times, most recently from 4439c09 to 8a76c2b Compare July 13, 2024 10:07
@petermetz
Copy link
Contributor

@jagpreetsinghsasan Good job on reducing the code duplication! I ran it through the copy paste detection and we are down 2% overall for the besu package compared to the main branch!

jscpd-report-main.zip

jscpd-report-feature963c.zip

Screenshot from 2024-07-13 11-33-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.

@jagpreetsinghsasan LGTM now, but please

  1. fix the linter errors and also
  2. sneak in a fix for the test case packages/cactus-plugin-ledger-connector-besu/src/test/typescript/integration/plugin-ledger-connector-besu/deploy-contract/private-deploy-contract-from-json-cactus.test.ts where the container image pull of the ledger was accidentally messed up by someone (most likely me) in a different commit and it's failing because of that. Just search in that file for start(true) and then replace it with just start() so that it does not skip the downloading of the image from the container registry. This is not a change that is related to this PR, but I'm hoping that if you do make that change then the besu connector tests will start passing overall.

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-963c branch 2 times, most recently from 6558ff0 to 019fd84 Compare July 16, 2024 04:57
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
Copy link
Contributor Author

Hi @petermetz , I have included the changes you have requested already
image

@petermetz
Copy link
Contributor

@jagpreetsinghsasan Oh, sorry, my bad. I just saw that the test was failing again but now for it's a different error. 🥲

@petermetz petermetz self-requested a review July 18, 2024 12:50
    Primary Changes
    ---------------
    1. Updated besu connector to remove hard
       dependency on keychain

    Changes required to incorporate 1)
    ----------------------------------
    2. Updated openapi.tpl.json to have non-keychain
       endpoints
    3. Generated code and updated web-services for them
    4. Updated transact( ) and deployContract( ) fx
    5. Added deployContractNoKeychain( ) fx

Fixes hyperledger-cacti#963

Signed-off-by: jagpreetsinghsasan <[email protected]>
@petermetz petermetz merged commit f5b60b4 into hyperledger-cacti:main Jul 18, 2024
142 of 143 checks passed
@petermetz petermetz deleted the feature-963c branch July 18, 2024 13:23
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): remove hard dependency on keychain
3 participants