-
Notifications
You must be signed in to change notification settings - Fork 209
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
Conversation
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]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #448 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 279 279
Lines 14995 15074 +79
=========================================
+ Hits 14995 15074 +79
Continue to review full report at Codecov.
|
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
|
||
if existing { | ||
var existingBlockchainIDs fftypes.FFStringArray | ||
_ = transactionRows.Scan(&existingBlockchainIDs) |
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 we check for error?
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 took the approach that a problem here would be detected below, but it might have been lazy
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.
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 { |
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 think the blockchain ID is now inside the blockchain.Event
, right? so no need to pass it separately also
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.
Sorry I missed this one
pkg/tokens/plugin.go
Outdated
@@ -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 |
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 should not be needed (here or on TokenTransfer below) since it's been included in blockchain.Event
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.
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!
Signed-off-by: Andrew Richardson <[email protected]>
blockchainIds
on thetransaction
objectUpsertTransaction
with lower-case only text, and no duplicatesblockNumber/transactionIndex/eventIndex/subEventNumber
convention to all event IDs000000654321/000010/00002
would be:654321
10
2
subEventNumber
is reserved for protocols that batch logical events (such as transfers) in an data array emitted by a single eventTransaction example from E2E:
To which the following Blockchain event is associated:
Event example from E2E: