-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[RFC] Add native function create_uuid #8401
Changes from 1 commit
2cd733c
3decce0
c9b68ec
4c4ef51
fc9b372
2284e55
2f0d2eb
80b7cbb
4d7c564
b79d4bc
631d66d
bdfaa55
90f30db
98ad018
0a00af6
f02858f
b632592
1c444d4
3a616ba
f89370c
83c4a21
8a6ff98
bf29748
ee02708
638e2af
331e728
ce78b04
575e409
0e7206d
16b52a4
f1afb1e
29e0b62
5308088
9b1591b
0424d8b
ad6f336
ea7fbb6
c267a23
7568b15
f180aa0
23f8776
fb9bce1
82fec8a
41cb9af
4df1c1c
752caae
8cdf218
545283d
2cadd2c
aad09cc
2b40d44
06aa5b9
584cca2
16252d8
77b2002
8fc34f6
6cdd260
53e4d3d
c3f1a1a
6d7b0dd
987f88f
29725d6
b86849f
18b4e3b
e863b87
b5aecf0
e2e05cf
1e71ba9
03b9fea
b54fa0c
3f0d40d
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 |
---|---|---|
@@ -1,4 +1,7 @@ | ||
module aptos_framework::transaction_context { | ||
/// Return a globally unique identifier | ||
public native fun create_guid(): address; | ||
|
||
/// Return the script hash of the current entry function. | ||
public native fun get_script_hash(): vector<u8>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,10 @@ | |
use crate::natives::helpers::{make_safe_native, SafeNativeContext, SafeNativeResult}; | ||
use aptos_types::on_chain_config::{Features, TimedFeatures}; | ||
use better_any::{Tid, TidAble}; | ||
use move_core_types::gas_algebra::InternalGas; | ||
use move_core_types::{account_address::AccountAddress, gas_algebra::InternalGas}; | ||
use move_vm_runtime::native_functions::NativeFunction; | ||
use move_vm_types::{loaded_data::runtime_types::Type, values::Value}; | ||
use sha2::{Digest, Sha256}; | ||
use smallvec::{smallvec, SmallVec}; | ||
use std::{collections::VecDeque, fmt::Debug, sync::Arc}; | ||
|
||
|
@@ -15,15 +16,20 @@ use std::{collections::VecDeque, fmt::Debug, sync::Arc}; | |
/// natives of this extension. | ||
#[derive(Tid)] | ||
pub struct NativeTransactionContext { | ||
txn_hash: Vec<u8>, | ||
/// This is the number of GUIDs (Globally unique identifiers) issued during the execution of this transaction | ||
guid_counter: u64, | ||
script_hash: Vec<u8>, | ||
chain_id: u8, | ||
} | ||
|
||
impl NativeTransactionContext { | ||
/// Create a new instance of a native transaction context. This must be passed in via an | ||
/// extension into VM session functions. | ||
pub fn new(script_hash: Vec<u8>, chain_id: u8) -> Self { | ||
pub fn new(txn_hash: Vec<u8>, script_hash: Vec<u8>, chain_id: u8) -> Self { | ||
Self { | ||
txn_hash, | ||
guid_counter: 0, | ||
script_hash, | ||
chain_id, | ||
} | ||
|
@@ -34,6 +40,40 @@ impl NativeTransactionContext { | |
} | ||
} | ||
|
||
/*************************************************************************************************** | ||
* native fun create_guid | ||
* | ||
* gas cost: base_cost | ||
* | ||
**************************************************************************************************/ | ||
#[derive(Clone, Debug)] | ||
pub struct CreateGUIDGasParameters { | ||
pub base: InternalGas, | ||
} | ||
|
||
fn native_create_guid( | ||
gas_params: &CreateGUIDGasParameters, | ||
context: &mut SafeNativeContext, | ||
mut _ty_args: Vec<Type>, | ||
_args: VecDeque<Value>, | ||
) -> SafeNativeResult<SmallVec<[Value; 1]>> { | ||
context.charge(gas_params.base)?; | ||
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 wonder if we can somehow use the charge formula from sha256 hashing instead as this is effectively just a hash operation? cc @alinush 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. You could, sure! Just pass those SHA256 gas parameter along as an extra parameter when the native is created. We have an example of this in the 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. This would be a cleaner solution and would also address Alin's comment on the redundant gas schedule entry 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 talked to @vgao1996 today about both the options. He said "Both would work. Although I'd personally prefer the new gas parameter approach to decouple create_uuid from sha256." 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. Yeah, my concern was is 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. @alinush Ah I see your point. Again, I'm fine with either approach. Was suggesting the decoupling more from a general cleaness perspective, but if you prefer reusing the gas parameters, then just go ahead for it. 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. Well, I defer to you if you think reusing gas parameters might NOT be a good idea. My first instinct is that duplicating gas parameters is likely to lead to forgetting to update them in all the places. Which is why I think we should re-use them.... 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. Is the point here that we should be charging here cost of SHA2-256 + extra ? is that possible to specify, and have only extra be configurable? 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. @alinush With the updated code, then 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. Correct, but like @igor-aptos is saying below, the |
||
|
||
let mut transaction_context = context | ||
.extensions_mut() | ||
.get_mut::<NativeTransactionContext>(); | ||
transaction_context.guid_counter += 1; | ||
let mut hash_arg = Vec::new(); | ||
hash_arg.extend(transaction_context.txn_hash.clone()); | ||
vusirikala marked this conversation as resolved.
Show resolved
Hide resolved
|
||
hash_arg.extend(transaction_context.guid_counter.to_le_bytes().to_vec()); | ||
let hash_vec = Sha256::digest(hash_arg.as_slice()).to_vec(); | ||
Ok(smallvec![Value::address(AccountAddress::new( | ||
hash_vec | ||
.try_into() | ||
.expect("Unable to convert hash vector into [u8]") | ||
))]) | ||
} | ||
|
||
/*************************************************************************************************** | ||
* native fun get_script_hash | ||
* | ||
|
@@ -67,22 +107,34 @@ fn native_get_script_hash( | |
#[derive(Debug, Clone)] | ||
pub struct GasParameters { | ||
pub get_script_hash: GetScriptHashGasParameters, | ||
pub create_guid: CreateGUIDGasParameters, | ||
} | ||
|
||
pub fn make_all( | ||
gas_params: GasParameters, | ||
timed_features: TimedFeatures, | ||
features: Arc<Features>, | ||
) -> impl Iterator<Item = (String, NativeFunction)> { | ||
let natives = [( | ||
"get_script_hash", | ||
make_safe_native( | ||
gas_params.get_script_hash, | ||
timed_features, | ||
features, | ||
native_get_script_hash, | ||
let natives = [ | ||
( | ||
"get_script_hash", | ||
make_safe_native( | ||
gas_params.get_script_hash, | ||
timed_features.clone(), | ||
features.clone(), | ||
native_get_script_hash, | ||
), | ||
), | ||
( | ||
"create_guid", | ||
make_safe_native( | ||
gas_params.create_guid, | ||
timed_features, | ||
features, | ||
native_create_guid, | ||
), | ||
), | ||
)]; | ||
]; | ||
|
||
crate::natives::helpers::make_module_natives(natives) | ||
} |
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.
Or provide
get_tx_hash()
andguid_counter()
native functions, and implement thecreate_guid()
in MoveThere 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.
If we go that route, then we can explore multiple solutions and leave it more to the user to dictate their preferred approach.
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.
Isn't it generally preferable to provide as simple interface as possible, and reduce the cognitive load on the smart contract developer? In terms of functionality, the developer would achieve the same with both the options, but the developer has to put more effort into how to construct GUIDs if we expose
get_tx_hash
andguid_counter
without an additional benefit.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.
The
get_tx_hash
is helpful in many scenarios,guid_counter
may be just for thecreate_guid
. And I think thenew_table_handle
is repeated withcreate_guid
; if we do not reuse theguid_counter
, it will produce the repeattable_handle
andguid
.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.
@jolestar - more details on the choice : https://www.notion.so/aptoslabs/Move-Parallelism-for-MetaPixel-982afa7c10ca494fad235fd1aa4318f4?pvs=4#d87bb42cc7a648d39fa3e699a4fffd35
guid_counter is scoped separately, while table_handle isn't