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.

Sign up for GitHub

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

Jump to bottom

[#3992][Events] Extract amount field from coins into TransferObject event #4328

Merged
merged 5 commits into from
Sep 2, 2022
Merged

[#3992][Events] Extract amount field from coins into TransferObject event #4328

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ad4d02
Add amount field to (Sui)Event::TransferObject. Populate for Transfe…
velvia Aug 26, 2022
a456cd2
Extract the transferred amount from all transfer TX types
velvia Aug 29, 2022
2970354
Fix tests. They check amount from transfers is recorded in Events! :)
velvia Aug 29, 2022
fce3e97
Fix sui.yaml for format test
velvia Sep 1, 2022
f24cf9b
Fix test and type in TS
velvia Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/sui-adapter/src/adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use move_vm_runtime::{native_functions::NativeFunctionTable, session::Serialized
use sui_framework::EventType;
use sui_types::{
base_types::*,
coin::Coin,
error::ExecutionError,
error::{ExecutionErrorKind, SuiError},
event::{Event, TransferType},
Expand Down Expand Up @@ -798,6 +799,13 @@ fn handle_transfer<
_ => None,
};
if let Some(type_) = transfer_type {
// Check for the transfer amount if the object is a Coin
let amount = if Coin::is_coin(&move_obj.type_) {
let coin = Coin::from_bcs_bytes(move_obj.contents())?;
Some(coin.value())
} else {
None
};
state_view.log_event(Event::TransferObject {
package_id: ObjectID::from(*module_id.address()),
transaction_module: Identifier::from(module_id.name()),
Expand All @@ -806,6 +814,7 @@ fn handle_transfer<
object_id: obj_id,
version: old_obj_ver,
type_,
amount,
})
}
} else {
Expand Down
8 changes: 8 additions & 0 deletions crates/sui-cluster-test/src/helper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ pub struct TransferObjectEventChecker {
object_id: Option<ObjectID>,
version: Option<SequenceNumber>,
type_: Option<TransferType>,
amount: Option<u64>,
}

impl TransferObjectEventChecker {
Expand Down Expand Up @@ -196,6 +197,11 @@ impl TransferObjectEventChecker {
self
}

pub fn amount(mut self, amount: u64) -> Self {
self.amount = Some(amount);
self
}

pub fn check(self, event: &SuiEvent) {
if let SuiEvent::TransferObject {
package_id,
Expand All @@ -205,6 +211,7 @@ impl TransferObjectEventChecker {
object_id,
version,
type_,
amount,
} = event
{
assert_eq_if_present!(self.package_id, package_id, "package_id");
Expand All @@ -218,6 +225,7 @@ impl TransferObjectEventChecker {
assert_eq_if_present!(self.object_id, object_id, "object_id");
assert_eq_if_present!(self.version, version, "version");
assert_eq_if_present!(self.type_, type_, "type_");
assert_eq!(self.amount, *amount);
} else {
panic!("event {:?} is not TransferObject Event", event);
}
Expand Down
21 changes: 18 additions & 3 deletions crates/sui-core/src/execution_engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")),
Expand All @@ -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(())
Expand All @@ -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");
Expand Down Expand Up @@ -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?
Copy link
Contributor

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?

Copy link
Contributor Author

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.

amount: transferred,
});
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.


#[cfg(debug_assertions)]
assert_eq!(object.version(), version);
Expand Down
36 changes: 19 additions & 17 deletions crates/sui-core/tests/staged/sui.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,65 +136,67 @@ ExecutionFailureStatus:
8:
InvalidTransferSuiInsufficientBalance: UNIT
9:
NonEntryFunctionInvoked: UNIT
InvalidCoinObject: UNIT
10:
EntryTypeArityMismatch: UNIT
NonEntryFunctionInvoked: UNIT
11:
EntryTypeArityMismatch: UNIT
12:
EntryArgumentError:
NEWTYPE:
TYPENAME: EntryArgumentError
12:
13:
CircularObjectOwnership:
NEWTYPE:
TYPENAME: CircularObjectOwnership
13:
14:
MissingObjectOwner:
NEWTYPE:
TYPENAME: MissingObjectOwner
14:
15:
InvalidSharedChildUse:
NEWTYPE:
TYPENAME: InvalidSharedChildUse
15:
16:
InvalidSharedByValue:
NEWTYPE:
TYPENAME: InvalidSharedByValue
16:
17:
TooManyChildObjects:
STRUCT:
- object:
TYPENAME: ObjectID
17:
18:
InvalidParentDeletion:
STRUCT:
- parent:
TYPENAME: ObjectID
- kind:
OPTION:
TYPENAME: DeleteKind
18:
19:
InvalidParentFreezing:
STRUCT:
- parent:
TYPENAME: ObjectID
19:
PublishErrorEmptyPackage: UNIT
20:
PublishErrorNonZeroAddress: UNIT
PublishErrorEmptyPackage: UNIT
21:
PublishErrorDuplicateModule: UNIT
PublishErrorNonZeroAddress: UNIT
22:
SuiMoveVerificationError: UNIT
PublishErrorDuplicateModule: UNIT
23:
MovePrimitiveRuntimeError: UNIT
SuiMoveVerificationError: UNIT
24:
MovePrimitiveRuntimeError: UNIT
25:
MoveAbort:
TUPLE:
- TYPENAME: ModuleId
- U64
25:
VMVerificationOrDeserializationError: UNIT
26:
VMVerificationOrDeserializationError: UNIT
27:
VMInvariantViolation: UNIT
ExecutionStatus:
ENUM:
Expand Down
6 changes: 6 additions & 0 deletions crates/sui-json-rpc-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1758,6 +1758,7 @@ pub enum SuiEvent {
object_id: ObjectID,
version: SequenceNumber,
type_: TransferType,
amount: Option<u64>,
},
/// Delete object
#[serde(rename_all = "camelCase")]
Expand Down Expand Up @@ -1825,6 +1826,7 @@ impl SuiEvent {
object_id,
version,
type_,
amount,
} => SuiEvent::TransferObject {
package_id,
transaction_module: transaction_module.to_string(),
Expand All @@ -1833,6 +1835,7 @@ impl SuiEvent {
object_id,
version,
type_,
amount,
},
Event::DeleteObject {
package_id,
Expand Down Expand Up @@ -1918,6 +1921,7 @@ impl PartialEq<SuiEvent> for Event {
type_: self_type,
object_id: self_object_id,
version: self_version,
amount: self_amount,
} => {
if let SuiEvent::TransferObject {
package_id,
Expand All @@ -1927,6 +1931,7 @@ impl PartialEq<SuiEvent> for Event {
object_id,
version,
type_,
amount,
} = other
{
package_id == self_package_id
Expand All @@ -1936,6 +1941,7 @@ impl PartialEq<SuiEvent> for Event {
&& self_object_id == object_id
&& self_version == version
&& self_type == type_
&& self_amount == amount
} else {
false
}
Expand Down
46 changes: 23 additions & 23 deletions crates/sui-open-rpc/samples/objects.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,21 @@
"fields": {
"description": "An NFT created by the Sui Command Line Tool",
"id": {
"id": "0xed50d0e9db53a2c9cfc4f0f4c7639fa63785f7e8"
"id": "0x1e8e405f6fb3e6f7566cfbd2f05a112ce81024a7"
},
"name": "Example NFT",
"url": "ipfs://bafkreibngqhl3gaa7daob4i2vccziay2jjlp435cf66vhono7nrvww53ty"
}
},
"owner": {
"AddressOwner": "0x2d08997bd97399b19039dda9a00595d059b50bd4"
"AddressOwner": "0x226c0b18b03eaebee3db1a2e4613c29eddb216e6"
},
"previousTransaction": "isMsSeqggHGpjP4jfjmCdUzuQqXH2K/d4LwILffRW3Y=",
"previousTransaction": "2ELWlyt4F1JkZvg83l6G1lZnrwRy9A6XOy/fqidfOHs=",
"storageRebate": 25,
"reference": {
"objectId": "0xed50d0e9db53a2c9cfc4f0f4c7639fa63785f7e8",
"objectId": "0x1e8e405f6fb3e6f7566cfbd2f05a112ce81024a7",
"version": 1,
"digest": "Anb5B9SULTxWoJ8j5bB5RTg5eo/F683jgWncYOxDlHc="
"digest": "pgFtd238r5ahpT5DHYFu8+8iqNJxOP6a9rnJIwbScvA="
}
}
},
Expand All @@ -37,19 +37,19 @@
"fields": {
"balance": 100000000,
"id": {
"id": "0x0bb17115dde7633421736a7618d3be02336b7c25"
"id": "0x02ed45cab5c1f60e26b77cca50054e12280d624b"
}
}
},
"owner": {
"AddressOwner": "0x2d08997bd97399b19039dda9a00595d059b50bd4"
"AddressOwner": "0x226c0b18b03eaebee3db1a2e4613c29eddb216e6"
},
"previousTransaction": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
"storageRebate": 0,
"reference": {
"objectId": "0x0bb17115dde7633421736a7618d3be02336b7c25",
"objectId": "0x02ed45cab5c1f60e26b77cca50054e12280d624b",
"version": 0,
"digest": "Nh5q/N12r6r0W+4y2M1u/bJvM5aKDT+/R3uWmZEMtb8="
"digest": "DYGVPcOLk7d0FSbMEsAQUUwx9tQAGDF9qlls2Aix2TQ="
}
}
},
Expand All @@ -59,16 +59,16 @@
"data": {
"dataType": "package",
"disassembled": {
"m1": "// Move bytecode v5\nmodule c859504b7eb1baf927ea7c0f980908dada2c840a.m1 {\nstruct Forge has store, key {\n\tid: UID,\n\tswords_created: u64\n}\nstruct Sword has store, key {\n\tid: UID,\n\tmagic: u64,\n\tstrength: u64\n}\n\ninit(Arg0: &mut TxContext) {\nB0:\n\t0: CopyLoc[0](Arg0: &mut TxContext)\n\t1: Call[6](new(&mut TxContext): UID)\n\t2: LdU64(0)\n\t3: Pack[0](Forge)\n\t4: StLoc[1](loc0: Forge)\n\t5: MoveLoc[1](loc0: Forge)\n\t6: MoveLoc[0](Arg0: &mut TxContext)\n\t7: FreezeRef\n\t8: Call[7](sender(&TxContext): address)\n\t9: Call[0](transfer<Forge>(Forge, address))\n\t10: Ret\n}\npublic magic(Arg0: &Sword): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Sword)\n\t1: ImmBorrowField[0](Sword.magic: u64)\n\t2: ReadRef\n\t3: Ret\n}\npublic strength(Arg0: &Sword): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Sword)\n\t1: ImmBorrowField[1](Sword.strength: u64)\n\t2: ReadRef\n\t3: Ret\n}\nentry public sword_create(Arg0: &mut Forge, Arg1: u64, Arg2: u64, Arg3: address, Arg4: &mut TxContext) {\nB0:\n\t0: MoveLoc[4](Arg4: &mut TxContext)\n\t1: Call[6](new(&mut TxContext): UID)\n\t2: MoveLoc[1](Arg1: u64)\n\t3: MoveLoc[2](Arg2: u64)\n\t4: Pack[1](Sword)\n\t5: StLoc[5](loc0: Sword)\n\t6: MoveLoc[5](loc0: Sword)\n\t7: MoveLoc[3](Arg3: address)\n\t8: Call[1](transfer<Sword>(Sword, address))\n\t9: CopyLoc[0](Arg0: &mut Forge)\n\t10: ImmBorrowField[2](Forge.swords_created: u64)\n\t11: ReadRef\n\t12: LdU64(1)\n\t13: Add\n\t14: MoveLoc[0](Arg0: &mut Forge)\n\t15: MutBorrowField[2](Forge.swords_created: u64)\n\t16: WriteRef\n\t17: Ret\n}\nentry public sword_transfer(Arg0: Sword, Arg1: address) {\nB0:\n\t0: MoveLoc[0](Arg0: Sword)\n\t1: MoveLoc[1](Arg1: address)\n\t2: Call[1](transfer<Sword>(Sword, address))\n\t3: Ret\n}\npublic swords_created(Arg0: &Forge): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Forge)\n\t1: ImmBorrowField[2](Forge.swords_created: u64)\n\t2: ReadRef\n\t3: Ret\n}\n}"
"m1": "// Move bytecode v5\nmodule 33c8739d658297e0aa49d5ed26b1c4be94dd47fd.m1 {\nstruct Forge has store, key {\n\tid: UID,\n\tswords_created: u64\n}\nstruct Sword has store, key {\n\tid: UID,\n\tmagic: u64,\n\tstrength: u64\n}\n\ninit(Arg0: &mut TxContext) {\nB0:\n\t0: CopyLoc[0](Arg0: &mut TxContext)\n\t1: Call[6](new(&mut TxContext): UID)\n\t2: LdU64(0)\n\t3: Pack[0](Forge)\n\t4: StLoc[1](loc0: Forge)\n\t5: MoveLoc[1](loc0: Forge)\n\t6: MoveLoc[0](Arg0: &mut TxContext)\n\t7: FreezeRef\n\t8: Call[7](sender(&TxContext): address)\n\t9: Call[0](transfer<Forge>(Forge, address))\n\t10: Ret\n}\npublic magic(Arg0: &Sword): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Sword)\n\t1: ImmBorrowField[0](Sword.magic: u64)\n\t2: ReadRef\n\t3: Ret\n}\npublic strength(Arg0: &Sword): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Sword)\n\t1: ImmBorrowField[1](Sword.strength: u64)\n\t2: ReadRef\n\t3: Ret\n}\nentry public sword_create(Arg0: &mut Forge, Arg1: u64, Arg2: u64, Arg3: address, Arg4: &mut TxContext) {\nB0:\n\t0: MoveLoc[4](Arg4: &mut TxContext)\n\t1: Call[6](new(&mut TxContext): UID)\n\t2: MoveLoc[1](Arg1: u64)\n\t3: MoveLoc[2](Arg2: u64)\n\t4: Pack[1](Sword)\n\t5: StLoc[5](loc0: Sword)\n\t6: MoveLoc[5](loc0: Sword)\n\t7: MoveLoc[3](Arg3: address)\n\t8: Call[1](transfer<Sword>(Sword, address))\n\t9: CopyLoc[0](Arg0: &mut Forge)\n\t10: ImmBorrowField[2](Forge.swords_created: u64)\n\t11: ReadRef\n\t12: LdU64(1)\n\t13: Add\n\t14: MoveLoc[0](Arg0: &mut Forge)\n\t15: MutBorrowField[2](Forge.swords_created: u64)\n\t16: WriteRef\n\t17: Ret\n}\nentry public sword_transfer(Arg0: Sword, Arg1: address) {\nB0:\n\t0: MoveLoc[0](Arg0: Sword)\n\t1: MoveLoc[1](Arg1: address)\n\t2: Call[1](transfer<Sword>(Sword, address))\n\t3: Ret\n}\npublic swords_created(Arg0: &Forge): u64 {\nB0:\n\t0: MoveLoc[0](Arg0: &Forge)\n\t1: ImmBorrowField[2](Forge.swords_created: u64)\n\t2: ReadRef\n\t3: Ret\n}\n}"
}
},
"owner": "Immutable",
"previousTransaction": "Be64kF6cWEYihWgwYgzKvyMZNixU5JRKBWvqgiagB1s=",
"previousTransaction": "OpmWAAt95ttoaSlhcim8NEn/2fhOQbaWzljP1ozB2aI=",
"storageRebate": 0,
"reference": {
"objectId": "0xc859504b7eb1baf927ea7c0f980908dada2c840a",
"objectId": "0x33c8739d658297e0aa49d5ed26b1c4be94dd47fd",
"version": 1,
"digest": "yKbnwGKWpCEiK0UaxQlOXxoTGvNUvc+KJONSVt+a9QE="
"digest": "SfTD5O4aBKYVN81rnGoe0NEBp8TazXFVbeitlVMT7Pg="
}
}
},
Expand All @@ -77,21 +77,21 @@
"details": {
"data": {
"dataType": "moveObject",
"type": "0xeaa96afeecd3cf700ca68d6cad4af93f05b59bcd::hero::Hero",
"type": "0x7c9b26e2c8ddff67419256af26fe5f1eddcf0fa7::hero::Hero",
"has_public_transfer": true,
"fields": {
"experience": 0,
"game_id": "0x3e2f9cf92b994dd41d1eed32a05095bcb97ba403",
"game_id": "0x0b074aadeb6657f11e8da9b14dde1b943479d22f",
"hp": 100,
"id": {
"id": "0x1f45933e11d09fa15e65ba6735ce4205c3deec24"
"id": "0x5653a665c3b63f686f480fe3dd2c38ea8a15ed7c"
},
"sword": {
"type": "0xeaa96afeecd3cf700ca68d6cad4af93f05b59bcd::hero::Sword",
"type": "0x7c9b26e2c8ddff67419256af26fe5f1eddcf0fa7::hero::Sword",
"fields": {
"game_id": "0x3e2f9cf92b994dd41d1eed32a05095bcb97ba403",
"game_id": "0x0b074aadeb6657f11e8da9b14dde1b943479d22f",
"id": {
"id": "0xefda1238171072e9dda0d1e2a20e584d399e4a93"
"id": "0xfe0a8b7af6af8b87a4a942bd4c80de288f4a4a50"
},
"magic": 10,
"strength": 1
Expand All @@ -100,14 +100,14 @@
}
},
"owner": {
"AddressOwner": "0x2d08997bd97399b19039dda9a00595d059b50bd4"
"AddressOwner": "0x226c0b18b03eaebee3db1a2e4613c29eddb216e6"
},
"previousTransaction": "l2NgFk9pnshb2qn/14mlS4B63rc+2BKLgktEuyyk4M8=",
"previousTransaction": "uPtcs0LiTGhOqSiJFVfWBPaicQGF4a6rBvgHdH3JAbw=",
"storageRebate": 21,
"reference": {
"objectId": "0x1f45933e11d09fa15e65ba6735ce4205c3deec24",
"objectId": "0x5653a665c3b63f686f480fe3dd2c38ea8a15ed7c",
"version": 1,
"digest": "JLfV8R0HBVKX0BVGvzxP+Fzi/ghbZLkObAIzm0vGoLU="
"digest": "OYmwHOxXeJrmzBxlDpxCWRz3V8RD8hQ5vcR9Kzc1yzk="
}
}
}
Expand Down
Loading