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

Fabric plugin #184

Merged
merged 21 commits into from Oct 14, 2021
Merged

Fabric plugin #184

merged 21 commits into from Oct 14, 2021

Conversation

jimthematrix
Copy link
Contributor

@jimthematrix jimthematrix commented Sep 1, 2021

The plugin provides:

  • BatchPin transactions and events support
  • Identity support based on the new identity manager implementation by Add identity manager and API input matrix logic #192
    • the plugin helps translating the short-form of an identity key such as user1 to the fully expanded form that is consistent to the representation used by the FireFly Chaincode such as myMSP::x509::CN=user1,OU=client::CN=fabric-ca-server. The conversion is based on the local MSP configurations of the downstream fabconnect microservice

What About Tokens?

Tokens support for a Fabric based token contract implementation will be added in a future PR. This will need some careful planning and hopefully soliciting a wide range of opinions from the community in order to first determine:

  • should we have an UTXO or Account/Balance based implementation?
  • which specific implementation?

Unlike the tokens specs and implementations in the Ethereum ecosystem, which has high quality libraries like OpenZepplin that captures contract implementations already proven in the mainnet deployments, the Fabric ecosystem has not yet produced clear standards or proven implementations.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #184 (c038a47) into main (77dbb16) will decrease coverage by 0.20%.
The diff coverage is 91.95%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #184      +/-   ##
===========================================
- Coverage   100.00%   99.79%   -0.21%     
===========================================
  Files          199      201       +2     
  Lines        10859    11141     +282     
===========================================
+ Hits         10859    11118     +259     
- Misses           0       17      +17     
- Partials         0        6       +6     
Impacted Files Coverage Δ
internal/blockchain/fabric/fabric.go 91.48% <91.48%> (ø)
internal/blockchain/fabric/config.go 100.00% <100.00%> (ø)
internal/events/batch_pin_complete.go 100.00% <100.00%> (ø)
internal/events/persist_batch.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77dbb16...c038a47. Read the comment docs.

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2021

Codecov Report

Merging #184 (e2c747d) into main (c894700) will decrease coverage by 0.46%.
The diff coverage is 84.21%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #184      +/-   ##
===========================================
- Coverage   100.00%   99.53%   -0.47%     
===========================================
  Files          214      217       +3     
  Lines        11820    12181     +361     
===========================================
+ Hits         11820    12124     +304     
- Misses           0       42      +42     
- Partials         0       15      +15     
Impacted Files Coverage Δ
internal/blockchain/fabric/certs.go 54.00% <54.00%> (ø)
internal/blockchain/fabric/fabric.go 88.62% <88.62%> (ø)
internal/blockchain/fabric/config.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17bd391...e2c747d. Read the comment docs.

jimthematrix and others added 19 commits October 12, 2021 09:58
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
@nguyer
Copy link
Contributor

nguyer commented Oct 13, 2021

Why are there solidity files in this PR? Was that from files moving around, and maybe things got duplicated?

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

A couple of questions, but otherwise looks good to me

Contexts: hashes,
}
input := newTxInput(pinInput)
res, err := f.invokeContractMethod(ctx, f.defaultChannel, f.chaincode, signingKey, operationID.String(), input, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this always be using f.defaultChannel? Or is the plan to revisit this once the concept of multiple ledgers is built out in FireFly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, support for the ledger construct needs to be worked on in the future. I believe there are some work related to tokens to allow one ledger to be used for messages and another ledger for tokens, that @awrichar may be getting to soon. That should help teasing out the proper support for ledgers in FireFly, at which point we should consider adding the proper channel support in the Fabric plugin.

At the moment, since there's only a single ledger in FireFly, it'll always be mapped to the default channel in the Fabric plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I can attempt to clarify the difference between plugins, ledgers, and ordering contexts. With regards to plugins, we make no assumptions about any of them sharing a particular chain or contract - so the plugins for blockchain-pinned messages and for tokens are treated separately and can be some combination (such as messaging on Fabric and tokens on Ethereum ERC1155). This also implies that the ordering contexts for token events and messaging events are separate (but related), and the implications of that are still being sorted on #245.

Ledgers are a further grouping of visibility within a particular blockchain plugin (basically corresponding to channels on Fabric). Currently there is no support for ledgers in the token plugin.

@jimthematrix
Copy link
Contributor Author

Why are there solidity files in this PR? Was that from files moving around, and maybe things got duplicated?

thanks for catching that, must have been my mishandling during rebase, will remove that in a commit

Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
@jimthematrix jimthematrix merged commit 3d9f3ad into hyperledger:main Oct 14, 2021
@jimthematrix jimthematrix deleted the fabric-plugin branch October 14, 2021 18:12
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.

None yet

4 participants