[#3992][Events] Extract amount field from coins into TransferObject event #4328
Conversation
|
@666lcz @longbowlu @patrickkuo I have now added amounts to events for transfers of any coins. |
| version: object.version(), | ||
| type_: TransferType::Coin, // Should this be a separate type, like SuiCoin? | ||
| amount: transferred, | ||
| }); |
There was a problem hiding this comment.
The PR description says you're just adding a field, but it looks like you're logging a whole new event. Is it correct that this was never emitted before?
This comment is a bit beyond the scope of this PR, but I also have some concerns about the format of this event - most of this data is redundant with existing transaction data, yet we are adding it all to the core data store for what may be one of the most common types of events. Can't we synthesize this data later, not on the validators, when we are emitting events?
There was a problem hiding this comment.
I've updated the PR title. Yes an event was not emitted before for TransferSui transactions, that was an oversight.
most of this data is redundant with existing transaction data, yet we are adding it all to the core data store for what may be one of the most common types of events
So a couple of things here.... there were a bunch of discussions about the best place for this to go, some of it might be in the issue linked in the description, but the consensus was that we would put it in an event. Yes, the object version is elsewhere in the effects, but not the transfer type and the amount. The transfer type cannot be deduced from effects for Move transactions, for example, and the amount is hidden in the Coin object layout itself. The destination of the transfer also is not directly derivable, though you could get it out of looking up the new object owner.
I agree that some of this data is redundant, and could be derived later, but this is really a larger discussion. One consideration is that it is really important that events be part of the historical data record -- partners and users will want to look up events, especially this amount field -- and if the effects from the validator do not include the full events, then we have a discrepancy between the state of the validator and full nodes. This discrepancy makes coming up with a full set of data for a checkpoint much more complicated.
I would not worry too much about redundant data and data storage costs right now. The long term data storage will probably look nothing like it does today.
velvia
changed the title
| recipient: Owner::AddressOwner(recipient), | ||
| object_id: object.id(), | ||
| version: object.version(), | ||
| type_: TransferType::Coin, // Should this be a separate type, like SuiCoin? |
There was a problem hiding this comment.
Can you resolve this comment? Not sure if I understand this question correctly, but this event is emitted for non-sui coins(e.g., coin::Coin) right?
There was a problem hiding this comment.
Oh. Maybe it's a good question for you. I can just remove the comment, the question was whether we need a different transfer type for Sui coins vs non-Sui coins. That code is in transfer_sui() which is I believe only for Sui coins?
So the question is do we need a separate transfer type for Sui coins vs non Sui 'coin::Coin` types. For now I just leave it the same.
There was a problem hiding this comment.
cc @666lcz , do we need changes in TS-SDK?
This is not a breaking change, so please feel free to land as it. The change on the ts side is just add a one-line changeamount: number | null to
sui/sdk/typescript/src/types/events.ts
Line 30 in 0b2605b
|
@666lcz thanks, I'll try including that one line change while I'm fixing some other tests. |
Sui implementation for #3992
This adds an amount field to the
TransferObjectevent plus implementation in theEventStoreand relevant data structures. It is currently stored in the JSON field alongside transfer type and object version.Currently (in the PR) there are three paths for the creation of a TransferObject event:
TransferObjecttransactions, inexecution_engine::transfer_object(), at the end we calllog_eventto create the eventTransferSuitransactions, I added alog_event()call at the end ofexecution_engine::transfer_sui()with the amountlog_event()is called for various kinds of transfers, as well as new object creations.This PR will deserialize the Move object for paths 1 and 3 to get the amount. Thus, any coin that is transferred will have the amount recorded in the
TransferObjectevent now.