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

Pass through full structure of receipt events for detailed reporting #72

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Aug 10, 2022

With EVMConnect we pass through lots of really important information in the updates to a transaction (aka "replies" or "receipts" on the WebSocket), as it's being shepherded through nonce/gas management, and confirmations, to a blockchain (public, or private).

The ERC20/ERC721 connector is currently masking all this info, and turning it into a binary success: true + message field, which has two problems:

  1. We lose all the rich detail when FireFly Core stores it in the operation
  2. The TransactionUpdate gets interpreted as Failure, rather than just an informational update that leaves the operation in Pending

This PR tries to preserve the existing fields, so that the new tokens connector can still be used with FireFly V1.0 + EthConnect, as well as FireFly V1.1 + EthConnect or EVMConnect

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@dechdev dechdev merged commit c4d1223 into hyperledger:main Aug 10, 2022
@awrichar awrichar deleted the full-receipts branch August 10, 2022 14:44
@awrichar
Copy link
Contributor

the new tokens connector can still be used with FireFly V1.0

I'm not sure this is something we're trying to preserve. It may already be broken for all I know (we certainly don't test that combination). I would personally vote to change the format completely and drop legacy fields. We specifically have a release-1.0 branch of this repository if we need to make releases that are compatible with 1.0, but we are not guaranteeing mainline to remain compatible with 1.0.

Also, these changes will need to be duplicated to erc1155.

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.

3 participants