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

Fit Fixed16 balance from sidechain in decimal response #122

Closed
alexvanin opened this issue Oct 23, 2020 · 4 comments · Fixed by #127
Closed

Fit Fixed16 balance from sidechain in decimal response #122

alexvanin opened this issue Oct 23, 2020 · 4 comments · Fixed by #127
Assignees
Labels
bug Something isn't working
Milestone

Comments

@alexvanin
Copy link
Contributor

No description provided.

@cthulhu-rider
Copy link
Contributor

Found that if the balance exceeds maximum int64 value, we receive stack item with IntegerT and cut int64 value (e.g. 923 * 10 ^ 16 -> -9216744073709551616.

At the same time IntFromStackItem utility function calls Int64 method, that returns undefined value if integer overflows int64. As a solution, we can return *big.Int value from IntFromStackItem function.

@alexvanin
Copy link
Contributor Author

Found that if the balance exceeds maximum int64 value, we receive stack item with IntegerT and cut int64 value (e.g. 923 * 10 ^ 16 -> -9216744073709551616.

At the same time IntFromStackItem utility function calls Int64 method, that returns undefined value if integer overflows int64. As a solution, we can return *big.Int value from IntFromStackItem function.

While it is correct, the roots of the issue are a bit deeper.

First of all, both neo-go and neo runtimes are capable to work with 256-bit integers. However, both of them use JSON to encode and decode invocation arguments and stack items of invocation result. In RFC 7159 section 6 there is a recommendation to encode numbers in range [-(2**53)+1, (2**53)-1].

Neo node throws error if the number is out of this bound, while neo-go node can return any big.Int number. However neo-go node can't pass big.Int number as an invocation parameter, it works only with int64 numbers.


Back to the balance contract. Now it has Fixed16 precision. While it can correctly process Fixed16 inside the contract, balanceOf and transfer methods can't return or get a number bigger than 2e53-1. In decimal integer values it will be:

# Max decimal integer value
Fixed8:  90 071 992
Fixed12: 9 007
Fixed16: 9

With lower precision, we can limit the max size of a single Deposit or Withdraw transaction so it will produce transfer invocation within the bound. However several Deposit still can accumulate quite a big number to break the bound.

Neo-go can return big.Int values on balanceOf invocation. This happens at accounting.Balance neofs-api RPC call. However neofs-api defines response of this RPC as a decimal message with int64 value. Unfortunately Fixed12 number can be bigger than maxInt64 and this is the case @cthulhu-rider pointed in the previous message.


There are next steps to fix this issue by lowering the impact of RFC 7159 limitation:

  1. Reduce precision of balance contract from Fixed16 to Fixed12. It is still an open question of what precision is required for a balance contract.
  2. Made 9000 GAS limit for single Deposit and Withdraw operation in neofs-contract.
  3. Check overflow in neofs-node accounting wrapper.
    Getting an accounting balance is a simple information call and it doesn't use in any NeoFS operations. All balance-related operations are invoked by contracts except Deposit and Withdraw which will have an internal limit (see 2). There are several cases for neofs-node to parse balanceOf result:
  • Success
    • if less than maxInt64, return it as it is (can work with both neo-go and neo RPC nodes)
    • if bigger than maxInt64, lower precision to fit value in int64 field and return it (only with neo-go RPC node)
  • Fail
    • return error (out of bound for neo RPC node or internal \ network errors for both)

@cthulhu-rider
Copy link
Contributor

I don't think we have a problem with int64 overflow on NeoFS rpc side because we transfer funds amount as Decimal number. We can refactor internal functions to return big.Int values, and return bigIntToDecimal() from accounting.Balance rpc.

@alexvanin
Copy link
Contributor Author

We can refactor internal functions to return big.Int values, and return bigIntToDecimal() from accounting.Balance rpc.

Yes, that what we need to do in step 3. Implement some bigIntToDecimal() function and fit big.Int into Decimal. However it will work only with neo-go RPC node, while neo node will return error if value is bigger than 2**53-1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants