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

[RFC] Add native function create_uuid #8401

Merged
merged 71 commits into from
Jun 26, 2023
Merged

[RFC] Add native function create_uuid #8401

merged 71 commits into from
Jun 26, 2023

Conversation

vusirikala
Copy link
Contributor

@vusirikala vusirikala commented May 26, 2023

Description

Add native function create_uuid to NativeTransactionContext. This function will create a universally unique identifier by hashing SessionId || uuid_counter. These universally unique identifiers are helpful when assigning universally unique names for objects in Token V2 standard.

Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need move changes to be able to test it end to end

check how other native functions are tested, and potentially add a test there - i.e. that calling this on different transactions or multiple times in the same transaction, it gives different results.

aptos-move/framework/src/natives/transaction_context.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff. Overall the code matches a lot of what I expected. Could we push this to an AIP quickly to discuss how we might migrate from the current account guid concept to the transaction guid?

Furthermore, it makes me wonder in what circumstances we might be blocked on really considering to use this. For example, events would still benefit from either aggregators or eliminating the sequence numbers all together. Objects could immediately make use of this. Though one could argue then the object could be further improved by eliminating the legacy GUID field from object core.

@vusirikala
Copy link
Contributor Author

you'll need move changes to be able to test it end to end

check how other native functions are tested, and potentially add a test there - i.e. that calling this on different transactions or multiple times in the same transaction, it gives different results.

Added a unit test that creates 50 GUIDs in a transaction and verifies that all of them are unique.

@vusirikala vusirikala requested a review from vgao1996 as a code owner May 27, 2023 23:38
@davidiw
Copy link
Contributor

davidiw commented May 27, 2023

Another thing to contemplate. We already have a framework for defining GUIDs as an address plus an integer. Would it make more sense to follow that paradigm rather than define a new paradigm and have to port everything over? I'm a little stuck on the hassle it would be to introduce a new GUID type.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get alignment on what a proper next-gen guid is prior to landing this.

davidiw

This comment was marked as duplicate.

@@ -1,4 +1,7 @@
module aptos_framework::transaction_context {
/// Return a globally unique identifier
public native fun create_guid(): address;
Copy link
Contributor

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() and guid_counter() native functions, and implement the create_guid() in Move

Copy link
Contributor

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.

Copy link
Contributor Author

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 and guid_counter without an additional benefit.

Copy link
Contributor

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 the create_guid. And I think the new_table_handle is repeated with create_guid; if we do not reuse the guid_counter, it will produce the repeat table_handle and guid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geekflyer
Copy link
Contributor

geekflyer commented May 28, 2023

if we introduce a new API I would use this as opportunity to call this UUID (instead of GUID), which is the much more common term used in other programming languages and libs.

@vusirikala
Copy link
Contributor Author

Another thing to contemplate. We already have a framework for defining GUIDs as an address plus an integer. Would it make more sense to follow that paradigm rather than define a new paradigm and have to port everything over? I'm a little stuck on the hassle it would be to introduce a new GUID type.

I have made a few changes in object.move to utilize the new create_guid method. What do you think?

@alinush
Copy link
Contributor

alinush commented May 28, 2023

Oh, btw, please make an effort to ensure that GUID hashes are domain separated from everything else in Rust and Move. See "Current domain-separation for hashing and signatures" in Notion.

@vusirikala vusirikala changed the title Add native function create_guid [RFC] Add native function create_guid May 30, 2023
@vusirikala vusirikala changed the title [RFC] Add native function create_guid [RFC] Add native function create_uuid May 31, 2023
* Add create_token method in token.move

* add deprecated flag to create_token_from_account

* Add test case and rename to create

* Add feature flag condition in test case

* enabled feature flag in the unit test

* Enabling auid feature flag in ambassador unit tests
@@ -83,6 +83,7 @@ spec aptos_framework::staking_config {
}

spec calculate_and_save_latest_epoch_rewards_rate(): FixedPoint64 {
pragma verify_duration_estimate = 120;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday, I got some move prover test errors in Github actions. I think Junkil added this to fix those errors.

aptos-move/framework/aptos-framework/sources/object.move Outdated Show resolved Hide resolved
}

/// Return the transaction hash of the current transaction
public native fun get_txn_hash(): vector<u8>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though we are not using it, does this need to be gated?

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 couldn't really think of a reason to gate it.

@vusirikala vusirikala enabled auto-merge (squash) June 26, 2023 21:59
@vusirikala vusirikala disabled auto-merge June 26, 2023 22:00
@vusirikala vusirikala enabled auto-merge (squash) June 26, 2023 22:47
@vusirikala vusirikala requested a review from davidiw June 26, 2023 22:54
@igor-aptos igor-aptos disabled auto-merge June 26, 2023 22:59
@igor-aptos igor-aptos dismissed davidiw’s stale review June 26, 2023 23:00

Stale review, changes addressed

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@vusirikala vusirikala enabled auto-merge (squash) June 26, 2023 23:15
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672

Compatibility test results for aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672 (PR)
1. Check liveness of validators at old version: aptos-node-v1.4.4
compatibility::simple-validator-upgrade::liveness-check : committed: 6776 txn/s, latency: 4925 ms, (p50: 4900 ms, p90: 6800 ms, p99: 8100 ms), latency samples: 230400
2. Upgrading first Validator to new version: 3f0d40dd34459ceb4aa9dbf88678635752755672
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3905 txn/s, latency: 8194 ms, (p50: 8900 ms, p90: 9400 ms, p99: 10000 ms), latency samples: 148420
3. Upgrading rest of first batch to new version: 3f0d40dd34459ceb4aa9dbf88678635752755672
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2836 txn/s, latency: 11011 ms, (p50: 10900 ms, p90: 16200 ms, p99: 16900 ms), latency samples: 107780
4. upgrading second batch to new version: 3f0d40dd34459ceb4aa9dbf88678635752755672
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6064 txn/s, latency: 5457 ms, (p50: 5600 ms, p90: 8100 ms, p99: 9300 ms), latency samples: 212260
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 3f0d40dd34459ceb4aa9dbf88678635752755672

two traffics test: inner traffic : committed: 6019 txn/s, latency: 6480 ms, (p50: 6300 ms, p90: 7800 ms, p99: 11100 ms), latency samples: 2612562
two traffics test : committed: 100 txn/s, latency: 2495 ms, (p50: 2400 ms, p90: 2900 ms, p99: 3300 ms), latency samples: 1880
Max round gap was 1 [limit 4] at version 1326785. Max no progress secs was 3.682085 [limit 10] at version 1326785.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672

Compatibility test results for aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672 (PR)
Upgrade the nodes to version: 3f0d40dd34459ceb4aa9dbf88678635752755672
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4634 txn/s, latency: 6998 ms, (p50: 7200 ms, p90: 9500 ms, p99: 9800 ms), latency samples: 171480
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> 3f0d40dd34459ceb4aa9dbf88678635752755672 passed
Test Ok

@vusirikala vusirikala merged commit b0b041c into main Jun 26, 2023
@vusirikala vusirikala deleted the satya/create-guid branch June 26, 2023 23:48
gedigi pushed a commit that referenced this pull request Aug 2, 2023
* Add native function create_guid

* Append a constant to hash_arg

* Add move test to verify uniqueness of guids

* Charging gas for create_guid method

* Add gas for create guid

* Add create_guid to object.move

* Change GUID to UUID

* Change GUID to UUID in tests

* Change GUID to UUID in tests

* Add create_object method in object.move

* Fix typo

* Fix typo

* fix trigger condition for build jobs

* Add native_get_txn_hash

* Add native_get_txn_hash

* Deprecate the old create_object_from_object and create_object_from_account

* Bumped the latest_gas_version to 10

* Bumped gas version to 10

* Changed gas feature version to 9

* Moved transaction hash hashing to authenticator.rs

* Add documentation to create_uuid method

* Use AuthenticationKey struct for create_uuid

* Remove deprecated comment

* Updatd comments in object.move

* Changed create_uuid to use only one hash

* Add create_unique_address function

* Add drop, store capabilities to UUID

* Resetting the create_object_from_account function

* Add comment for create_object function

* feature gating

* deprecated tag

* Add transaction context spec

* Add a test case in transaction_context

* Updated move test case

* Add create_token method

* Update test cases

* rust lint

* Changed comments

* Remove create_token method

* Add feature flag for test

* fix a unit test

* Fix unit test

* moved unit test to object.move

* changing uuid to auid

* rust lint

* Minor changes

* Change uuid to auid in tests

* Change uuid to auid in tests

* changed name to generate_unique_address

* Fixed the specs which timeout

* Fixed a unit test

* Shift test from e2e-move-tests to transaction_context.move

* Add a comment in object.move

* rust lint

* Add create_token method in token.move (#8825)

* Add create_token method in token.move

* add deprecated flag to create_token_from_account

* Add test case and rename to create

* Add feature flag condition in test case

* enabled feature flag in the unit test

* Enabling auid feature flag in ambassador unit tests

* Enable auid flag in an ambassador.move unit test

* Changed gas cost

* changed name from generate_unique_address to generate_auid_address

* Updated a comment

* Feature gate get_txn_hash

* changing names in test cases

---------

Co-authored-by: geekflyer <[email protected]>
Co-authored-by: Junkil Park <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants