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

Fix account balance overflow #377

Merged
merged 7 commits into from
Mar 20, 2024
Merged

Conversation

JimmyFate
Copy link
Contributor

@JimmyFate JimmyFate commented Mar 12, 2024

Closes #110

Usage related changes

  • amount must be a string instead of plain integer in /mint

Development related changes

Checklist:

  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • Performed code self-review
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Linked the issues which this PR resolves
  • Checked the TODO section in README.md if this PR resolves it
  • Updated the tests
  • All tests are passing - cargo test

These 2 tests are failing locally for me:

failures:
    test_messaging::can_deploy_l1_messaging_contract
    test_messaging::can_interact_with_l1

Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

  1. Why do we need a new dependency for all this (primitive_types::U256)? Can't we use the existing BigUint?
  2. I don't know if we want to introduce the breaking semantic change of requiring a string instead of a number in the /mint endpoint. Can't we deserialize a big numeric JSON literal into the desired numeric Rust type (be it U256 or BigUint), without it needing to be a string?

cc: @marioiordanov

@JimmyFate
Copy link
Contributor Author

  1. Why do we need a new dependency for all this (primitive_types::U256)? Can't we use the existing BigUint?

It'd require creating a custom U256 struct wrapping the BigUint, with other impls, bloating the codebase. I feel like it's better to just use an existing one.

  1. I don't know if we want to introduce the breaking semantic change of requiring a string instead of a number in the /mint endpoint. Can't we deserialize a big numeric JSON literal into the desired numeric Rust type (be it U256 or BigUint), without it needing to be a string?

Rust's built-in numeric types only go up to u128, so there's no built-in way to deserialize a u256 directly. I've you have any idea on how to do this, I'm down to change it.

@marioiordanov
Copy link
Contributor

marioiordanov commented Mar 13, 2024

  1. Why do we need a new dependency for all this (primitive_types::U256)? Can't we use the existing BigUint?

It'd require creating a custom U256 struct wrapping the BigUint, with other impls, bloating the codebase. I feel like it's better to just use an existing one.

  1. I don't know if we want to introduce the breaking semantic change of requiring a string instead of a number in the /mint endpoint. Can't we deserialize a big numeric JSON literal into the desired numeric Rust type (be it U256 or BigUint), without it needing to be a string?

Rust's built-in numeric types only go up to u128, so there's no built-in way to deserialize a u256 directly. I've you have any idea on how to do this, I'm down to change it.

It's possible for deserialising directly from some number. Just deserialize to serde_json::Value::Number. Then convert to string and then convert to U256

@FabijanC
Copy link
Contributor

It's possible for deserialising directly from some number. Just deserialize to serde_json::Value::Number. Then convert to string and then convert to U256

I ran an experiment and it seems that precision is lost that way: experiment link

@marioiordanov
Copy link
Contributor

experiment link

The experiment doesn't emulate starknet-devnet-rs, because it uses arbitrary precision in the crate that the file is

@FabijanC
Copy link
Contributor

FabijanC commented Mar 13, 2024

experiment link

The experiment doesn't emulate starknet-devnet-rs, because it uses arbitrary precision in the crate that the file is

Wow, didn't know that. I ran the code as a devnet test and indeed the precision is preserved.

@JimmyFate
Copy link
Contributor Author

It's possible for deserialising directly from some number. Just deserialize to serde_json::Value::Number. Then convert to string and then convert to U256

Thanks for the great tip @marioiordanov! Didn't know that.
Done as pointed out 👍

Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Can we now lose the U256 dependency?

@JimmyFate
Copy link
Contributor Author

Can we now lose the U256 dependency?

Could ethers be used instead or do you insist on a separate own struct for U256? starknet-devnet-core already uses it (it uses primitive-types itself too)

Copy link
Contributor

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Can we now lose the U256 dependency?

Could ethers be used instead or do you insist on a separate own struct for U256? starknet-devnet-core already uses it (it uses primitive-types itself too)

I meant losing the primitive-types dependency. But I guess an extra dependency is not such a big issue; your PR is good, so let's merge it. Thank you for the contribution and the patience.

@FabijanC FabijanC merged commit aab89fb into 0xSpaceShard:main Mar 20, 2024
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.

Trait Accounted treating balance in a wrong way
3 participants