Skip to content

[Rest API] [Txn Arg] support for signed int #17758

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
Open
wants to merge 1 commit into
base: jun/int_prover
Choose a base branch
from
Open

[Rest API] [Txn Arg] support for signed int #17758

wants to merge 1 commit into from
+627 −146

Conversation

junxzm1990
Copy link
Contributor

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@junxzm1990 Graphite App
Copy link
Contributor Author

junxzm1990 commented Oct 3, 2025 •
edited by wrwg
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@junxzm1990 junxzm1990 mentioned this pull request Oct 3, 2025
Open
16 tasks
@junxzm1990 junxzm1990 force-pushed the jun/sint-restapi branch 2 times, most recently from dd22f32 to 98a4711 Compare October 3, 2025 21:16
@junxzm1990 junxzm1990 marked this pull request as ready for review October 3, 2025 21:17
@junxzm1990 junxzm1990 changed the title [Rest API] support for signed int [Rest API] [Txn Arg] support for signed int Oct 3, 2025
georgemitenkov
georgemitenkov approved these changes Oct 5, 2025
View reviewed changes
api/types/src/derives.rs
StateKeyWrapper,
U64,
U128
U128,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why u256, u32 and u16 are not here?

aptos-move/aptos-sdk-builder/src/golang.rs
I32 => "sint32".into(),
I64 => "sint64".into(),
I128 => "serde.Sint128".into(),
I256 => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this expected, why u156 is also unimplemented I wonder, can we move them to unsupported / not allowed branch? This looks weird as below we seem to allow i/u256s...

ecosystem/indexer-grpc/indexer-grpc-fullnode/src/convert.rs
| MoveType::I128
| MoveType::I256 => {
//TODO()(#17645): support signed integers
unimplemented!("signed integers not yet supported");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this unparsable for now so that it is not possible to panic here?

ecosystem/indexer-grpc/indexer-grpc-fullnode/src/convert.rs
| MoveType::I128
| MoveType::I256 => {
//TODO()(#17645): support signed integers
unimplemented!("signed integers not yet supported");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, would None be enough?

third_party/move/move-binary-format/src/normalized.rs
U32,
#[serde(rename = "u256")]
U256,
// NOTE: Added in bytecode version v9, do not reorder!
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? This is legacy, it does not have function values either? The whole thing can be deleted but I think there are some dependencies somewhere...

third_party/move/tools/move-bytecode-utils/src/layout.rs
U64 => Format::U64,
U128 => Format::U128,
U256 => Format::TypeName(U256_SERDE_NAME.to_string()),
I8 => Format::I8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is build_normalized_type_layout used anywhere? I belive all that registry thing is just legacy code, I would vote to get rid of this instead 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgemitenkov georgemitenkov georgemitenkov approved these changes

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

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

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

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

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

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

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

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan 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.
2 participants
@junxzm1990 @georgemitenkov