-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[#3992][Events] Extract amount field from coins into TransferObject event #4328
Changes from all commits
9ad4d02
a456cd2
2970354
fce3e97
f24cf9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ use crate::authority::TemporaryStore; | |
use move_core_types::language_storage::ModuleId; | ||
use move_vm_runtime::{move_vm::MoveVM, native_functions::NativeFunctionTable}; | ||
use sui_adapter::adapter; | ||
use sui_types::coin::Coin; | ||
use sui_types::committee::EpochId; | ||
use sui_types::error::ExecutionError; | ||
use sui_types::gas::GasCostSummary; | ||
|
@@ -260,6 +261,8 @@ fn transfer_object<S>( | |
) -> Result<(), ExecutionError> { | ||
object.ensure_public_transfer_eligible()?; | ||
object.transfer_and_increment_version(recipient); | ||
// This will extract the transfer amount if the object is a Coin of some kind | ||
let amount = Coin::extract_balance_if_coin(&object)?; | ||
temporary_store.log_event(Event::TransferObject { | ||
package_id: ObjectID::from(SUI_FRAMEWORK_ADDRESS), | ||
transaction_module: Identifier::from(ident_str!("native")), | ||
|
@@ -268,6 +271,7 @@ fn transfer_object<S>( | |
object_id: object.id(), | ||
version: object.version(), | ||
type_: TransferType::Coin, | ||
amount, | ||
}); | ||
temporary_store.write_object(object); | ||
Ok(()) | ||
|
@@ -290,7 +294,7 @@ fn transfer_sui<S>( | |
#[cfg(debug_assertions)] | ||
let version = object.version(); | ||
|
||
if let Some(amount) = amount { | ||
let transferred = if let Some(amount) = amount { | ||
// Deduct the amount from the gas coin and update it. | ||
let mut gas_coin = GasCoin::try_from(&object) | ||
.expect("gas object is transferred, so already checked to be a SUI coin"); | ||
|
@@ -319,13 +323,24 @@ fn transfer_sui<S>( | |
// This is necessary for the temporary store to know this new object is not unwrapped. | ||
let newly_generated_ids = tx_ctx.recreate_all_ids(); | ||
temporary_store.set_create_object_ids(newly_generated_ids); | ||
Some(amount) | ||
} else { | ||
// If amount is not specified, we simply transfer the entire coin object. | ||
// We don't want to increment the version number yet because latter gas charge will do it. | ||
object.transfer_without_version_change(recipient); | ||
} | ||
Coin::extract_balance_if_coin(&object)? | ||
}; | ||
|
||
// TODO: Emit a new event type for this. | ||
temporary_store.log_event(Event::TransferObject { | ||
package_id: ObjectID::from(SUI_FRAMEWORK_ADDRESS), | ||
transaction_module: Identifier::from(ident_str!("native")), | ||
sender: tx_ctx.sender(), | ||
recipient: Owner::AddressOwner(recipient), | ||
object_id: object.id(), | ||
version: object.version(), | ||
type_: TransferType::Coin, // Should this be a separate type, like SuiCoin? | ||
amount: transferred, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the PR title. Yes an event was not emitted before for
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. |
||
|
||
#[cfg(debug_assertions)] | ||
assert_eq!(object.version(), version); | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.