-
Notifications
You must be signed in to change notification settings - Fork 227
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
Added cardano connector plugin #1636
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
========================================
Coverage 99.95% 99.96%
========================================
Files 339 342 +3
Lines 29780 30455 +675
========================================
+ Hits 29768 30443 +675
Misses 8 8
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! It's fantastic to see 👏🏼
Some initial thoughts and will follow up with more thorough review after playing around with it 😃
Address string `json:"address"` | ||
} | ||
|
||
var addressVerify = regexp.MustCompile("^addr1|^addr_test1|^stake1|^stake_test1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just used for (extremely simple) address validation. Cardano uses bech32 addresses with a conventional set of HRPs, but I wanted to avoid pulling in a bech32 decoder, so I allowlisted the conventional HRPs instead.
func (c *Cardano) DeployContract(ctx context.Context, nsOpID, signingKey string, definition, contract *fftypes.JSONAny, input []interface{}, options map[string]interface{}) (submissionRejected bool, err error) { | ||
body := map[string]interface{}{ | ||
"id": nsOpID, | ||
"contract": contract, | ||
"definition": definition, | ||
} | ||
var resErr common.BlockchainRESTError | ||
res, err := c.client.R(). | ||
SetContext(ctx). | ||
SetBody(body). | ||
SetError(&resErr). | ||
Post("/contracts/deploy") | ||
if err != nil || !res.IsSuccess() { | ||
return resErr.SubmissionRejected, common.WrapRESTError(ctx, &resErr, res, err, coremsgs.MsgCardanoconnectRESTErr) | ||
} | ||
return false, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the signing key being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. The "smart contracts" we're running in the cardano connector are offchain wasm components, nothing is actually published to the chain when you deploy a contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay need to understand this a bit better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the offline chat enough to explain why we don't need the signing key here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was! Going to test this e2e myself as well
func (c *Cardano) GenerateErrorSignature(ctx context.Context, event *fftypes.FFIErrorDefinition) string { | ||
// TODO: impl | ||
return "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interested to understand more how errors are reported in Cardano from smart contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cardano smart contract doesn't report errors right now, only events. I don't understand the difference between errors and events, and I couldn't find any code in other plugins which streamed errors from the connector.
I've copied the "event signature" code into this function, but that's just a guess at what it's supposed to do. I'd want to see how consumers actually use events to be more confident about that.
if !isBatch { | ||
var receipt common.BlockchainReceiptNotification | ||
_ = json.Unmarshal(msgBytes, &receipt) | ||
|
||
err := common.HandleReceipt(ctx, "", c, &receipt, c.callbacks) | ||
if err != nil { | ||
l.Errorf("Failed to process receipt: %+v", msgTyped) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is heavily inspired from FFTM and the evm plugin - I would recommend in the future moving to a durable event stream here where the receipts have to be ack'd back as part of a batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using EVM, Tezos, and FFTM as references for how the eventstream is supposed to work (trying my best to make Cardano match the semantics of other connectors). It looked like other connectors assume that every message in a batch is an event, not a receipt.
I think it makes sense to use resilient ack-ing for everything, but I'm a little wary about inventing semantics for it in this plugin that nothing else is using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - probably as a follow up. I'm building a framework at moment to allow for resilient ack-ing and get confirmation from FireFly core it has processed things correctly
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! A few comments 😃
Going to give it a go locally
const ( | ||
ReceiptTransactionSuccess string = "TransactionSuccess" | ||
ReceiptTransactionFailed string = "TransactionFailed" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to re-use these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not defined anywhere shared. You want I should move them to common
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I though they were, if you can move them to common would be great only if you have the bandwidth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
func (c *Cardano) DeployContract(ctx context.Context, nsOpID, signingKey string, definition, contract *fftypes.JSONAny, input []interface{}, options map[string]interface{}) (submissionRejected bool, err error) { | ||
body := map[string]interface{}{ | ||
"id": nsOpID, | ||
"contract": contract, | ||
"definition": definition, | ||
} | ||
var resErr common.BlockchainRESTError | ||
res, err := c.client.R(). | ||
SetContext(ctx). | ||
SetBody(body). | ||
SetError(&resErr). | ||
Post("/contracts/deploy") | ||
if err != nil || !res.IsSuccess() { | ||
return resErr.SubmissionRejected, common.WrapRESTError(ctx, &resErr, res, err, coremsgs.MsgCardanoconnectRESTErr) | ||
} | ||
return false, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was! Going to test this e2e myself as well
if (operation.Status == core.OpStatusPending || operation.Status == core.OpStatusInitialized) && txStatus != cardanoTxStatusPending { | ||
receipt := &common.BlockchainReceiptNotification{ | ||
Headers: common.BlockchainReceiptHeaders{ | ||
ReceiptID: statusResponse.GetString("id"), | ||
ReplyType: replyType, | ||
}, | ||
TxHash: statusResponse.GetString("transactionHash"), | ||
Message: statusResponse.GetString("errorMessage"), | ||
ProtocolID: receiptInfo.GetString("protocolId")} | ||
err := common.HandleReceipt(ctx, operation.Namespace, c, receipt, c.callbacks) | ||
if err != nil { | ||
log.L(ctx).Warnf("Failed to handle receipt") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like you have reliable receipts based on this https://github.com/hyperledger/firefly-cardano/blob/d42cee06819cec3f99bdef3d3b441cc3184a1cd3/firefly-cardanoconnect/src/routes/ws.rs#L71-L79 and https://github.com/hyperledger/firefly/pull/1636/files#diff-e1fde924db67e8e161a8976ebbf39c2e313ce393b0eda911c6f006d161a8452fR550-R554
So you don't need this hack that was done for FFTM based connectors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EnriqueL8 by "this hack" do you mean updating the status inside of GetTransactionStatus
? I can get rid of that, I just thought that it was a "better safe than sorry" redundancy which other connectors were using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
var receipt common.BlockchainReceiptNotification | ||
_ = json.Unmarshal(msgBytes, &receipt) | ||
|
||
err := common.HandleReceipt(ctx, namespace, c, &receipt, c.callbacks) | ||
if err != nil { | ||
l.Errorf("Failed to process receipt: %+v", msgTyped) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there no ack here? I couldn't find where you differentiate in the firefly-cardano
between a batch and this receipt. Would recommend this being durable please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EnriqueL8 this is the same logic (literally copypasted :) ) as the ethereum and fabric and tezos connectors. None of them are using acks.
We discussed this before: #1636 (comment)
I'm open to making receipts durable, but I don't want to invent the semantics for it in this single connector if it's something that all connectors should be doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've recently added a new function to the operation callback handler for durable receipts but I understand the complexity of making acking work in the cardano connector itself with receipts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is we are actively trying to remove this in the other plugins, so it's debt that would need be tackled for the Cardano to be truly robust for receipt listening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a tool available, I'll try to use it
@@ -0,0 +1,47 @@ | |||
// Copyright © 2025 Kaleido, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update all copyrights to be the company you work for in the new files added
func (nm *networkMap) generateCardanoAddressVerifier(identity *core.Identity, verifier *core.Verifier) *VerificationMethod { | ||
return &VerificationMethod{ | ||
ID: verifier.Hash.String(), | ||
Type: "PaymentVerificationKeyShelley_ed25519", // hope that it's safe to assume we always use Shelley |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happenes when it's a script witness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can that happen? I thought DID documents were for identities attached to firefly nodes, i.e. the wallets they could spend from.
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
switch msgJSON.GetString("type") { | ||
case "ContractEvent": | ||
signature := msgJSON.GetString("signature") | ||
|
||
logger := log.L(ctx) | ||
logger.Infof("[%d:%d/%d]: '%s'", batchID, i+1, count, signature) | ||
logger.Tracef("Message: %+v", msgJSON) | ||
c.processContractEvent(ctx, namespace, events, msgJSON) | ||
case "Receipt": | ||
var receipt common.BlockchainReceiptNotification | ||
msgBytes, _ := json.Marshal(msgMap) | ||
_ = json.Unmarshal(msgBytes, &receipt) | ||
|
||
err := common.HandleReceipt(ctx, namespace, c, &receipt, c.callbacks) | ||
if err != nil { | ||
log.L(ctx).Errorf("Failed to process receipt: %+v", msgMap) | ||
} | ||
default: | ||
log.L(ctx).Errorf("Unexpected message in batch: %+v", msgMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks!
Error from build - FYI @onelapahead I believe you added recently this DX callback. Feels like a race condition as sometimes fails and others passes
|
Alright fixed above #1658 |
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was reminded yesterday of the key being mandatory today as part of deploying a contract and it doesn't make sense for Cardano as the connector has used that API to run a off-chain dApp that is responsible of building Cardano TX and indexing state. Raised a discussion on Discord https://discord.com/channels/905194001349627914/928377875827154984/1349704667524763689
|
||
## Create the stack | ||
|
||
A Cardano stack can be run in two different ways; with a firefly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Cardano stack can be run in two different ways; with a firefly | |
A Cardano stack can be run in two different ways with Hyperledger FireFly |
|
||
Start a local cardano node. The fastest way to do this is to [use mithril](https://mithril.network/doc/manual/getting-started/bootstrap-cardano-node/) to bootstrap the node. | ||
|
||
For an example of how to bootstrap and run the cardano node in docker, see [the firefly-cardano repo](https://github.com/hyperledger/firefly-cardano/blob/1be3b08d301d6d6eeb5b79e40cf3dbf66181c3de/infra/docker-compose.node.yaml#L4). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an example of how to bootstrap and run the cardano node in docker, see [the firefly-cardano repo](https://github.com/hyperledger/firefly-cardano/blob/1be3b08d301d6d6eeb5b79e40cf3dbf66181c3de/infra/docker-compose.node.yaml#L4). | |
For an example of how to bootstrap and run the Cardano node in Docker, see [the firefly-cardano repo](https://github.com/hyperledger/firefly-cardano/blob/1be3b08d301d6d6eeb5b79e40cf3dbf66181c3de/infra/docker-compose.node.yaml#L4). |
|
||
> **NOTE**: The cardano-node communicates over a Unix socket, so this will not work on Windows. | ||
|
||
Start a local cardano node. The fastest way to do this is to [use mithril](https://mithril.network/doc/manual/getting-started/bootstrap-cardano-node/) to bootstrap the node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start a local cardano node. The fastest way to do this is to [use mithril](https://mithril.network/doc/manual/getting-started/bootstrap-cardano-node/) to bootstrap the node. | |
Start a local Cardano node. The fastest way to do this is to [use mithril](https://mithril.network/doc/manual/getting-started/bootstrap-cardano-node/) to bootstrap the node. |
## Get some ADA | ||
|
||
Now that you have a stack, you need some seed funds to get started. Your stack was created with a wallet already (these are free to create in Cardano). To get the address, you can run | ||
```sh | ||
ff accounts list dev | ||
``` | ||
|
||
The response will look like | ||
```json | ||
[ | ||
{ | ||
"address": "addr_test1...", | ||
"privateKey": "..." | ||
} | ||
] | ||
``` | ||
|
||
If you're developing against a testnet such as preview, you can receive funds from the [testnet faucet](https://docs.cardano.org/cardano-testnets/tools/faucet). Pass the `address` from that response to the faucet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would recommend following this a Transaction either in this section or working with smart contracts section. I do see a gap of how do you create a simple transfer of ADA through the API
manifest.json
Outdated
"image": "ghcr.io/hyperledger/firefly-cardanoconnect", | ||
"tag": "v0.2.1", | ||
"sha": "de880330e10faee733a30306add75139ccdf248329724f0b1292cf62eb65619f" | ||
}, | ||
"cardanosigner": { | ||
"image": "hyperledger/firefly-cardanosigner", | ||
"tag": "main" | ||
"image": "ghcr.io/hyperledger/firefly-cardanosigner", | ||
"tag": "v0.2.1", | ||
"sha": "d277bb99de78ed0fafd8b6a83f78a70efaa493a2b2c8259a4c46508f6987b0d5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thank you
Sorry I tagged a comment in a linked PR as |
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Signed-off-by: Simon Gellis <[email protected]>
Proposed changes
Adds a cardano connector plugin to firefly core. This connector interacts with the firefly-cardanoconnect service from firefly-cardano.
Types of changes
Please make sure to follow these points
< issue name >
egAdded links in the documentation
.Other Information
The Cardano connector and implementation is still a work in progress; in particular we need documentation and tests. However, it works well enough to demo, and I'd like to merge this relatively soon so that people can run it without building firefly-core from a branch, or downloading an image from a private repo.