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

introduce the new integer types (u16, u32 & u256) #5626

Merged
merged 1 commit into from
Dec 9, 2022
Merged

introduce the new integer types (u16, u32 & u256) #5626

merged 1 commit into from
Dec 9, 2022

Conversation

vgao1996
Copy link
Contributor

@vgao1996 vgao1996 commented Nov 17, 2022 •
edited
Loading

Overview

Support for the missing integer types (u16, u32 & u256) has landed in upstream Move. This PR brings them into aptos-core and makes other Move dependencies up to date.

Backward Compatibility

This introduces a new gas schedule version (5) and a new feature flag VM_BINARY_FORMAT_V6. The VM will only start to accept the new binary format version if both conditions are met.

Work Items

  • Move fixes
  • api
  • sdk
    • ts
    • python
    • rust (broken, won't fix)
    • go (broken, won't fix)
  • versioning/feature gating
  • update bcs (won't land until this is addressed properly)
  • check if the cli works
  • check if the indexer works
  • more tests

This change is Reviewable

@vgao1996 vgao1996 changed the title introduce the new integer types (u16, u32 & u256) [WIP][very early draft] introduce the new integer types (u16, u32 & u256) Nov 18, 2022
gregnazario
gregnazario reviewed Nov 18, 2022
View reviewed changes
api/types/src/derives.rs Outdated
A string containing a 256-bit unsigned integer.

We represent u256 values as a string to ensure compatibility with languages such
as JavaScript that do not parse u64s in JSON natively.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note, this needs to be cleaned up for u64 -> u256

@vgao1996 vgao1996 force-pushed the u256 branch 13 times, most recently from 7db1901 to 7c6dd75 Compare December 2, 2022 22:57
@vgao1996 vgao1996 force-pushed the u256 branch 6 times, most recently from 4bfb359 to 81255df Compare December 6, 2022 19:39
@vgao1996 vgao1996 changed the title [WIP][very early draft] introduce the new integer types (u16, u32 & u256) introduce the new integer types (u16, u32 & u256) Dec 6, 2022
@vgao1996 vgao1996 marked this pull request as ready for review December 6, 2022 20:17
@vgao1996 vgao1996 disabled auto-merge December 8, 2022 22:00
@vgao1996 vgao1996 enabled auto-merge (squash) December 8, 2022 23:24
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@vgao1996 vgao1996 disabled auto-merge December 9, 2022 00:20
@vgao1996 vgao1996 enabled auto-merge (squash) December 9, 2022 00:23
gregnazario
gregnazario reviewed Dec 9, 2022
View reviewed changes
aptos-move/aptos-sdk-builder/src/golang.rs
U64 => "uint64".into(),
U128 => "serde.Uint128".into(),
U256 => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make a followup issue for U256 support for the go generator btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5837

ecosystem/typescript/sdk/src/transaction_builder/builder_utils.test.ts
@@ -143,7 +158,7 @@ describe("BuilderUtils", () => {
}).toThrow("Invalid type tag.");

expect(() => {
new TypeTagParser("u32").parseTypeTag();
new TypeTagParser("u3").parseTypeTag();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be u32 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.

This is intentional. It's a test trying to parse an invalid type tag.

crates/aptos/src/common/types.rs
Comment on lines +922 to +925

/// Specify the version of the bytecode the compiler is going to emit.
#[clap(long)]
pub bytecode_version: Option<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. By default, this flag is not set and the cli will tell the move compiler to use bytecode version 5 so to maintain compatibility with our current testnet and mainnet. The user can however, opt into version 6 using this flag if they want to try out the new integer types.

@vgao1996 vgao1996 disabled auto-merge December 9, 2022 00:30
@vgao1996 vgao1996 mentioned this pull request Dec 9, 2022
Closed
@vgao1996 vgao1996 enabled auto-merge (squash) December 9, 2022 00:40
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

✅ Forge suite land_blocking success on 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677

performance benchmark with full nodes : 6278 TPS, 6264 ms latency, 13900 ms p99 latency,(!) expired 1060 out of 2682160 txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7326 TPS, 5276 ms latency, 8900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677
compatibility::simple-validator-upgrade::single-validator-upgrade : 4416 TPS, 9317 ms latency, 12500 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677
compatibility::simple-validator-upgrade::half-validator-upgrade : 4450 TPS, 9491 ms latency, 12500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6255 TPS, 6316 ms latency, 10300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 0494d2b78e3dfb7d08e111740cd7ab0ee7c6b677 passed
Test Ok

@vgao1996 vgao1996 merged commit 496a2ce into aptos-labs:main Dec 9, 2022
areshand pushed a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
@vgao1996 @areshand
introduce the new integer types (u16, u32 & u256) (aptos-labs#5626)
72a4b30
@Markuze Markuze mentioned this pull request Dec 26, 2022
Closed
@Markuze Markuze mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregnazario gregnazario gregnazario left review comments

@CapCap CapCap CapCap left review comments

@alinush alinush alinush left review comments

@banool banool banool left review comments

@clay-aptos clay-aptos clay-aptos left review comments

@wrwg wrwg wrwg approved these changes

@movekevin movekevin movekevin approved these changes

@msmouse msmouse Awaiting requested review from msmouse msmouse is a code owner

@lightmark lightmark Awaiting requested review from lightmark lightmark is a code owner

@jjleng jjleng Awaiting requested review from jjleng

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@areshand areshand Awaiting requested review from areshand

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vgao1996 @gregnazario @CapCap @alinush @banool @wrwg @movekevin @clay-aptos