Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

New Feature: Hyperledger Integration (chaincode-fabric-evm) #2149

Merged
merged 33 commits into from Jul 10, 2019

Conversation

CruzMolina
Copy link
Contributor

@CruzMolina CruzMolina commented Jul 2, 2019

This PR builds on top of prior work in #1806 to implement chaincode-fabric-evm integration.

Example truffle-config.js for using Truffle with chaincode-fabric-evm:

module.exports = {
  networks: {
    development: {
      ...
      type: "fabric-evm"
      ...
    }
  }
};

@coveralls
Copy link

coveralls commented Jul 3, 2019

Coverage Status

Coverage increased (+0.02%) to 70.403% when pulling 5401704 on chaincode-fabric-evm into be4b664 on develop.

@CruzMolina CruzMolina changed the title New Feature: Hyperledger chaincode-fabric-evm Integration New Feature: Hyperledger Integration (chaincode-fabric-evm) Jul 3, 2019
@gnidan gnidan self-assigned this Jul 3, 2019
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

It's great that the override here is rather tiny! Thanks for all the tests to get things started with good practice.

Mostly my suggestions here are on code organization, since this brings us to three network types.

@gnidan gnidan removed their assignment Jul 3, 2019
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Looks good apart from the one comment!

packages/truffle-deployer/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

hooray!

async initNetworkType (web3: Web3Shim) {
// truffle has started expecting gas used/limit to be
// hex strings to support bignumbers for other ledgers
getBlock(web3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming note here, not to change for this PR, but we might want to fix at some point:

For explicitness purposes, these methods should probably include the word override or something similar, to convey that it's not that we're getting the block, but we're overriding how we get the block.

An alternative solution would be to abstract this, so that getBlock, getTransaction, would be in a mapping, something like:

const overrides = {
  "getBlock": (original) => {
    const _oldFormatter = original.method.outputFormatter;
    original.method.outputFormatter = (block) => {
      _oldFormatter.call(original, block);
      /* ... */
    };
  },
  /* ... */
}

Just if you want to keep this in the back of your mind for whenever.

@@ -1 +1,5 @@
export function getSupportedNetworks() {
return ["quorum", "fabric-evm"];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking approval, but I think we should fix the name here to reflect that this specifically relates to which network types use the legacy system. Maybe getLegacyNetworkTypes.

Ooh, we could take this a step further! We can add requiresLegacySystem as a boolean field on the Definition objects, and that way it becomes a lot more cohesive when adding a new network. Basically, just these steps:

  1. Create an overrides file + definition
  2. Import the definition and link it up in the web3-shim module

packages/truffle-interface-adapter/lib/web3-shim.ts Outdated Show resolved Hide resolved
@CruzMolina CruzMolina merged commit 4d0b93f into develop Jul 10, 2019
@CruzMolina CruzMolina deleted the chaincode-fabric-evm branch July 10, 2019 20:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants