Rectify Transactions and BlockchainEvents #408
Conversation
awrichar
requested review from
nguyer,
nickgaski and
peterbroadhurst
as code owners
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Blockchain-specific data has all moved to BlockchainEvent. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
A transfer will always be associated with a blockchain event, which may or may not be associated with a transaction. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
awrichar
force-pushed
the
transactions
branch
from
64ea833 to
0e654d2
Compare
|
Here's a diagram to describe the new relationships between Transactions, BlockchainEvents, and other items in FireFly: |
peterbroadhurst
left a comment
peterbroadhurst
left a comment
There was a problem hiding this comment.
This feels like a simplification throughout the code changes, and I'm very glad you were able to make the time to tackle this now. Thank you!
Couple of comments to consider.
| @@ -0,0 +1,10 @@ | |||
| BEGIN; | |||
| ALTER TABLE transactions ADD COLUMN ref UUID; | |||
| ALTER TABLE transactions ADD COLUMN signer VARCHAR(1024) NOT NULL; | |||
There was a problem hiding this comment.
Don't think the NOT NULL will work, as we've lost the data.
| @@ -0,0 +1,16 @@ | |||
| BEGIN; | |||
| CREATE TABLE blockchainevents ( | |||
There was a problem hiding this comment.
Think we need an index on tx_id for this one
There was a problem hiding this comment.
I see that the onchain-logic branch has these indexes now (they are on contractevents but I just renamed that table):
CREATE INDEX blockchainevents_name ON blockchainevents(namespace,name);
CREATE INDEX blockchainevents_timestamp ON blockchainevents(timestamp);
CREATE INDEX blockchainevents_subscription_id ON blockchainevents(subscription_id);
If we add 2 more indexes, is that a bit much for a potentially high-write-volume table?
There was a problem hiding this comment.
I do think the ones you mentioned are useful... I guess just wondering if I should also prune any others? Possibly name?
There was a problem hiding this comment.
Think we go with this for now, and test - can't see any obvious candidates to cull.
There was a problem hiding this comment.
In the end I did remove namespace,name. It's easy enough to add back later if we want.
| Event Event | ||
| } | ||
|
|
||
| type Event struct { |
There was a problem hiding this comment.
I think in our current codebase, this object might be better located at understand the difference nowfftypes.BlockchainEvent
| PoolProtocolID string | ||
|
|
||
| // Event contains info on the underlying blockchain event for this transfer | ||
| Event blockchain.Event |
There was a problem hiding this comment.
It feels slightly odd to me having the tokens plugin refer to the blockchain plugin (rather than exclusively fftypes).
However, I understand if blockchain.Event moved to fftypes it would need to be distinguished from fftypes.BlockchainEvent.
There was a problem hiding this comment.
Yea, I struggled a bit with where to define types. We have two versions of a lot of things - one is the shape of the raw object that pops out of the plugin, the other is the shape of the object built and stored by FireFly. For instance, blockchain.BatchPin and fftypes.Batch. I followed somewhat similar patterns with token types for pools/transfers (there is one struct in tokens and another in fftypes, and event manager does a mapping).
But blockchain.Event is now odd because it's describing the shape of a thing that is emitted in common from multiple plugins (blockchain and tokens). Couldn't decide where that should live, as it's fundamentally an internal "wire format" and didn't quite feel like it belongs in fftypes.
There was a problem hiding this comment.
I guess I have another option of defining a separate type as tokens.BlockchainEvent, and just giving it all the same fields as blockchain.Event.
There was a problem hiding this comment.
Let's go with this for now - journey makes sense
awrichar
force-pushed
the
transactions
branch
from
e894f55 to
93db38e
Compare
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
awrichar
force-pushed
the
transactions
branch
from
93db38e to
be94b48
Compare
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
peterbroadhurst
left a comment
peterbroadhurst
left a comment
There was a problem hiding this comment.
Thanks for the extra comments 👍
2 participants
Pre-requisite refactoring for #393
The goal is to split all blockchain-related information out of
Transaction, and store it in an associatedBlockchainEventobject.Transactionjust becomes a grouping construct for other objects (with no real data of its own), andBlockchainEventcontains all blockchain-ish data, including from internal events like "BatchPin," and from user-created subscriptions as well.