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

Add Blockchain Transaction IDs to FireFly Transaction objects and add blockchain IDs to events #448

Merged
merged 8 commits into from
Jan 27, 2022

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Jan 25, 2022

  • Adds a string-array field of blockchainIds on the transaction object
    • Backed by a comma-separated field
    • Alphabetically sorted on each UpsertTransaction with lower-case only text, and no duplicates
    • Updated when blockchain operations complete
    • Updated when blockchain events arrive that are correlated with a blockchain transaction
    • In all current FireFly transaction types, should be zero or one blockchain transactions
    • Future extensibility to more
  • Adds a structured blockNumber/transactionIndex/eventIndex/subEventNumber convention to all event IDs
    • Example 000000654321/000010/00002 would be:
      • Block number: 654321
      • Transaction index: 10
      • Log index (ethereum) / event number (fabric): 2
    • The convention with padding (12/6/6/6 zeros) is intended to provide an alphabetically sortable list
    • For Fabric this feature depends on fixing Inconsistent transactionId field name on receipts/events firefly-fabconnect#74
    • Tokens connectors are going to have tweaks to ensure the same number of zeros / separators for consistency
    • The final subEventNumber is reserved for protocols that batch logical events (such as transfers) in an data array emitted by a single event

Transaction example from E2E:

{
  "id": "264200de-188a-4aae-92e7-1e5345604345",
  "namespace": "default",
  "type": "batch_pin",
  "created": "2022-01-25T22:01:08.9671519Z",
  "status": "Succeeded",
  "blockchainIds": [
    "0xb819561e2149c6a77a1de8a971895e486e064f45da26e1c48d2b4263eccffc43"
  ]
}

To which the following Blockchain event is associated:

{
  "id": "8b85311c-b51f-4b69-bbbf-083ac4728db4",
  "sequence": 22,
  "source": "ethereum",
  "namespace": "default",
  "name": "BatchPin",
  "protocolId": "000000000031/000000/000000",
  "output": {
    "author": "0xce1fde1c97d3db88dbb83af16ac3b0efefeba80d",
    "batchHash": "0xb605c7d5238dd01fa594cab74db229a997e2c7c5150eeb43288ecdac30b62b97",
    "contexts": [
      "0x2a30597d4aca075c164bfb02c44de63802d66f1422295aa6b7ce8b0daa12c470"
    ],
    "namespace": "default",
    "payloadRef": "QmWC6JQnpoYAsWj6junfwaiq8DvQgoLBMQt2nkoaYmxzaZ",
    "timestamp": "1643148068",
    "uuids": "0x264200de188a4aae92e71e5345604345af60568cd62c48e69cadce0d498d0584"
  },
  "info": {
    "address": "0x3A4e59b55979d0D196aB39471DfA706b0a0CEce9",
    "blockNumber": "31",
    "logIndex": "0",
    "signature": "BatchPin(address,uint256,string,bytes32,bytes32,string,bytes32[])",
    "subId": "sb-51267be5-775c-4d22-6d72-b8a85675187b",
    "timestamp": "1643148068",
    "transactionHash": "0xb819561e2149c6a77a1de8a971895e486e064f45da26e1c48d2b4263eccffc43",
    "transactionIndex": "0x0"
  },
  "timestamp": "2022-01-25T22:01:08Z",
  "tx": {
    "type": "batch_pin",
    "id": "264200de-188a-4aae-92e7-1e5345604345"
  }
}

Event example from E2E:

awrichar and others added 4 commits January 25, 2022 09:22
Also move the query helpers from Contract Manager to Orchestrator, as this
has become a generic construct that spans more than one manager/plugin.

Signed-off-by: Andrew Richardson <[email protected]>
@peterbroadhurst peterbroadhurst marked this pull request as draft January 25, 2022 21:53
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #448 (296b428) into main (0ab0206) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #448   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          279       279           
  Lines        14995     15074   +79     
=========================================
+ Hits         14995     15074   +79     
Impacted Files Coverage Δ
internal/events/event_manager.go 100.00% <ø> (ø)
internal/blockchain/ethereum/ethereum.go 100.00% <100.00%> (ø)
internal/blockchain/fabric/fabric.go 100.00% <100.00%> (ø)
internal/broadcast/definition.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/transaction_sql.go 100.00% <100.00%> (ø)
internal/events/batch_pin_complete.go 100.00% <100.00%> (ø)
internal/events/blockchain_event.go 100.00% <100.00%> (ø)
internal/events/operation_update.go 100.00% <100.00%> (ø)
internal/events/token_pool_created.go 100.00% <100.00%> (ø)
internal/events/tokens_transferred.go 100.00% <100.00%> (ø)
... and 8 more

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 0ab0206...296b428. Read the comment docs.

@peterbroadhurst peterbroadhurst marked this pull request as ready for review January 25, 2022 22:05
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>

if existing {
var existingBlockchainIDs fftypes.FFStringArray
_ = transactionRows.Scan(&existingBlockchainIDs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check for error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... I took the approach that a problem here would be detected below, but it might have been lazy

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, actually seems ok

}
}

func (em *eventManager) confirmPool(ctx context.Context, pool *fftypes.TokenPool, ev *blockchain.Event) error {
func (em *eventManager) confirmPool(ctx context.Context, pool *fftypes.TokenPool, ev *blockchain.Event, blockchainTXID string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the blockchain ID is now inside the blockchain.Event, right? so no need to pass it separately also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this one

@@ -101,6 +101,9 @@ type TokenPool struct {
// Not guaranteed to be set for pool creation events triggered outside of FireFly
TransactionID *fftypes.UUID

// BlockchainTXID is the blockchain transaction hash that resulted in the event that created the pool
BlockchainTXID string
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed (here or on TokenTransfer below) since it's been included in blockchain.Event

Copy link
Contributor

@awrichar awrichar left a comment

Choose a reason for hiding this comment

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

Left a couple more comments, but marking approved now so you can merge after looking through those.

Thanks for taking this on; I think it alleviates a big chunk of the concerns/confusion over Transactions!

@awrichar awrichar merged commit 5526967 into hyperledger:main Jan 27, 2022
@awrichar awrichar deleted the blockchain-txids branch January 27, 2022 22:11
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