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

Explorer: Amount field for transferring coin #3992

Closed
666lcz opened this issue Aug 15, 2022 · 16 comments
Closed

Explorer: Amount field for transferring coin #3992

666lcz opened this issue Aug 15, 2022 · 16 comments

Comments

@666lcz
Copy link
Contributor

666lcz commented Aug 15, 2022

Figure out if it is possible to derive and store an amount field in transaction effects if:

  1. For TransferObject transaction where the object is a Coin
  2. For TransferSui transaction, where the amount is not specified.
@666lcz
Copy link
Contributor Author

666lcz commented Aug 16, 2022

potentially leverage the events

@sblackshear
Copy link
Collaborator

I think the inability to look at historical state without re-execution is a broader problem that might merit a broader solution. One fairly simple one is to support an optional version in get_object + have full nodes keep a limited amount of object history. I think keeping the history is as simple as flipping a DB config (@lxfind or @velvia would know)--defining the pruning policy would be the main thing.

@sblackshear
Copy link
Collaborator

This doesn't mean we shouldn't also look at adding amounts, but I do worry that if we support amounts for some kinds of coin transfers and not others (e.g., transfers in Move calls), folks will end up getting confused when money moves silently or appears to disappear.

@666lcz
Copy link
Contributor Author

666lcz commented Aug 23, 2022

but I do worry that if we support amounts for some kinds of coin transfers and not others (e.g., transfers in Move calls)

The proposal is to expose the amount for all the TransferObject event in Event store, is it possible to transfer an object without triggering an TransferObject event? If not, then this approach should cover for all Coin transfers.Let me know if I am missing anything.

cc @velvia @longbowlu

@velvia
Copy link
Contributor

velvia commented Aug 25, 2022

@666lcz @sblackshear a couple quick questions and then comments:

  1. Does anyone have the PR that added transfer amount in the code, or has it been there a long time?
  2. Just to confirm, both TransferObject and TransferSui are not MOVE operations. We don't have to worry about Move transfers that transfer coins? At least, if we could worry about that in a subsequent PR that would be great.

@sblackshear as far as inspecting historical state, you mean historical objects right? My long term plan is to include historical objects at the minimum at epoch boundaries, but ideally we can optionally carry the initial objects at the beginning of every checkpoint. Otherwise, when our TPS goes up, it will become impossible for anybody who is behind or new to catch up.

In the meantime, we are keeping older object versions and we don;'t have a purge policy yet. We can certainly have a policy that keeps a specified number of versions or some other criteria.

@666lcz
Copy link
Contributor Author

666lcz commented Aug 26, 2022

Does anyone have the PR that added transfer amount in the code, or has it been there a long time?

Transfer amount is only available for TransferSui transaction, which is specified by the user. We never expose transfer amount for TransferObject. But you can get the amount by looking at the balance field of the Move object.

Just to confirm, both TransferObject and TransferSui are not MOVE operations. We don't have to worry about Move transfers that transfer coins?

@velvia I am surprised to learn that Move transfers does not emit a TransferObject event. I think the event should be emitted for Move transfers because it's useful to know what objects get transferred and its associated amount for a given Move call transaction.(EDIT: I just confirmed with @longbowlu that we do emit TransferObject events for Move transfers)

@666lcz 666lcz self-assigned this Aug 26, 2022
@666lcz
Copy link
Contributor Author

666lcz commented Aug 26, 2022

In the meantime, we are keeping older object versions and we don;'t have a purge policy yet. We can certainly have a policy that keeps a specified number of versions or some other criteria.

@velvia nice, do you have a code pointer of how to fetch object by a version number? We can then add an optional version field to the existing getObject endpoint so that the client can query objects by version

@longbowlu
Copy link
Contributor

longbowlu commented Aug 26, 2022

I think the inability to look at historical state without re-execution is a broader problem that might merit a broader solution. One fairly simple one is to support an optional version in get_object + have full nodes keep a limited amount of object history. I think keeping the history is as simple as flipping a DB config (@lxfind or @velvia would know)--defining the pruning policy would be the main thing.

@sblackshear @666lcz currently we do keep historical objects. It's in the parent_sync table. Exposing an optional SequenceNumber on get_object is not hard , simply call this function.

In terms of confusion, to start, we only show amount for Sui objects by parsing them when generating events on FullNode. For non-Sui objects, it will be expensive to have parse all of their contents (structure may be complicated) and extract the amount on the node side. But we can add StructTag to the event so the client (e.g. explorer) can parse it by themselves. Note that for non-Sui objects, we don't (I think) support partial transfer, so it has to be the whole amount of the object.

@velvia
Copy link
Contributor

velvia commented Aug 26, 2022

@666lcz basically what @longbowlu said... we do emit TransferObject events for MOVE transfers. Sorry, what I mean is the TransferObject transaction type, not the event. The TransferObject transaction type means that it is not a MOVE Transaction type. I'm referring to the code in execute_transaction() in execution_engine.rs....

@666lcz
Copy link
Contributor Author

666lcz commented Aug 26, 2022

to start, we only show amount for Sui objects by parsing them when generating events on FullNode. For non-Sui objects, it will be expensive to have parse all of their contents (structure may be complicated) and extract the amount on the node side.

The balance field exists on the coin module instead of SUI. This issue is about adding amount for any transfer involving Coin<T>. Does your comment about "Expensive to parse" still apply?

what I mean is the TransferObject transaction type, not the event

The TransferObject transaction type also emits a transferobject event(example). So as long as we add the amount to transferobject event it should be fine. Is there anything tricky about constructing the transferobject event for TransferObject transaction type?

@velvia
Copy link
Contributor

velvia commented Aug 26, 2022

PR is out for Sui backend change: #4328

it only implements the partial TransferSui amount for now.

@velvia
Copy link
Contributor

velvia commented Aug 26, 2022

@666lcz

The TransferObject transaction type also emits a transferobject event(example). So as long as we add the amount to transferobject event it should be fine. Is there anything tricky about constructing the transferobject event for TransferObject transaction type?

The only tricky thing is that we would need to deserialize the object, just as Lu said. So we can match the object and if it comes from the Coin module then we deserialize it and get the amount field out, is that right?

@longbowlu
Copy link
Contributor

@velvia yes we probably can do that by looking into its StructTag (e.g. is it coin::Coin<sthsth>), which i hope can cover the majority of cases.

@lxfind
Copy link
Contributor

lxfind commented Aug 30, 2022

@sblackshear @velvia My concern for opening up a public API on the fullnode that allows for querying a historic version of an object is that, people might depend on it and expect it to work to a certain degree. It's difficult to define for how long we can keep historic object versions. For example, it is difficult to prune object state at epoch boundaries, because we simply don't have a good way to tell which object versions are older than the current epoch. We also cannot promise to keep objects for X versions, if we ever consider using lamport versions (which we will very likely given the dynamic child object loading work). I am also curious about the application: is there a similar use case from other chains? (e.g. be able to show account state from X transactions earlier)
Nodes syncing state to the latest deserves a dedicated solution, which might or might not involve having to keep individual historic object state. I don't think we have a design yet so I prefer to not consider that in the picture.
If we are adding this API to fullnode I recommend the name to be explicit to indicate that it's for debugging/testing purposes at the moment, until we have a rough sense on our object pruning strategy on fullnodes.

@velvia
Copy link
Contributor

velvia commented Aug 30, 2022

@lxfind

Nodes syncing state to the latest deserves a dedicated solution, which might or might not involve having to keep individual historic object state. I don't think we have a design yet so I prefer to not consider that in the picture.

yes and I can finish my design for this if needed. :)

My concern for opening up a public API on the fullnode that allows for querying a historic version of an object is that, people might depend on it and expect it to work to a certain degree.

I agree we have no guarantees on how long we keep stuff, nor should we. I agree with the concern. Maybe we should sketch out the requirements for something like older object lookup more thoroughly and then we can reassess.

Broadly speaking, the more successful Sui becomes, the harder it will be to keep any number of older versions (other than for bulk node state syncing, for example).

@velvia
Copy link
Contributor

velvia commented Sep 2, 2022

PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants