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

Large number handling #84

Closed
wbt opened this issue Jul 27, 2022 · 14 comments
Closed

Large number handling #84

wbt opened this issue Jul 27, 2022 · 14 comments

Comments

@wbt
Copy link

wbt commented Jul 27, 2022

Using the SDK, the results of a SELECT statement includes a rows object with an array of row arrays. If one of the selected values was an int type during table creation, it is present in the appropriate position of each row array as a JavaScript number.
This works fine in most cases.
However, my understanding is that the equivalent Solidity type is uint256.
One common use case would be to have a numeric integer primary key column of type uint256 and include a reference to this id in a second table as a foreign key reference, even if not explicitly declared.

Does Tableland's SDK actually store and return large numbers correctly? I was trying to build a test case to see what would be returned in the row value when a value was outside of JavaScript's integer range (specifically, I tried the number 18446744073709551616 (2^64) added into the query string without having to be stored numerically in JavaScript) and it looked like the change wasn't actually applied. Reading the docs, it looks like the type int64 might be more appropriate than uint256 as the EVN version of Tableland numbers, but even this has values well outside Javascript's safe range for accurate computation.

Other methods of interacting with blockchains tend to use some BigNumber implementation for numeric values input to or output from blockchain functions; this is one that does not. Should it?

@joewagner
Copy link
Contributor

Hey @wbt great question!
Your example of "integer primary key column of type uint256" isn't quite valid because uint256 is not an available column type in the Tableland SQL spec. See the docs for more detail https://docs.tableland.xyz/sql-specification#87bf6d3bab3e4ae7a9b7083da2bdff74
Also, the current maximum number of rows allowed was just increased from 10,000 to 100,000 so the limitations of large integers don't come into play in this case.

For your example of 2^64, this is larger than the largest allowed int in SQLite, which is 2^63 -1. This might be the reason your change wasn't applied. I would need more information to be sure.
In general, you can store integers between 2^53 -1 and 2^63 -1, but any javascript client won't be able to safely interpret the Validator's (application/json) response when they make read queries. If your client/app is using this sdk and you want to store numbers larger than Number.MAX_SAFE_INTEGER you could consider using a TEXT column type, or a BLOB column type. You would need to choose a strategy for how the TEXT/BLOB was formatted. Some examples of ways to store the Number 90071992547409911 would be:

  1. A JS String, e.g. '90071992547409911'
  2. An ethers.js BigNumber, e.g. { _hex: '0x013ffffffffffff7', _isBigNumber: true }

See the docs for more info on storing JSON https://docs.tableland.xyz/sql-specification#e2bee77cd6d04c679c1dbe1b2dd57204

@wbt
Copy link
Author

wbt commented Jul 27, 2022

Hmm, it looks like I may not have communicated my points very clearly. Hopefully this helps:

  • The demonstrations & examples (e.g. in CanvasGame) use uint256 as the correct type corresponding to int in Tableland. If that's not correct, the examples should use something more appropriate (int64 seems like the best fit) and this type correspondence should probably be made much clearer in the docs.
  • The maximum number of rows does come into play for the foreign key/id example, which means I picked a bad example. A more generic numeric column with a concept associated with some big numbers would've been more appropriate.
  • The discussion about the need to store numbers which could be larger than Number.MAX_SAFE_INTEGER in a text or blob column type really should be found in the docs with the description of the integer & numeric types.

@joewagner
Copy link
Contributor

@wbt thanks again for all of the input here.

  • The demonstrations & examples (e.g. in CanvasGame) use uint256 as the correct type corresponding to int in Tableland. If that's not correct, the examples should use something more appropriate (int64 seems like the best fit) and this type correspondence should probably be made much clearer in the docs.

Not sure I'm following what you mean on this point. The canvas game is not using this module. Can you provide a link to the place in the code you're referring to?

  • The discussion about the need to store numbers which could be larger than Number.MAX_SAFE_INTEGER in a text or blob column type really should be found in the docs with the description of the integer & numeric types.

We are actively working on the docs and I'll mention your idea to the team.

@wbt
Copy link
Author

wbt commented Jul 28, 2022

The canvas game is not using this module. Can you provide a link to the place in the code you're referring to?

On this line the type of columns id, x, and y are seen as int in Tableland, and on this line one can see that the equivalent Solidity type is uint256, apparently to the best knowledge of the authors of the example intending to show how Solidity works with Tableland. If the function doesn't accept such large parameter types, why would it specify that it does?

@joewagner
Copy link
Contributor

I guess I see what you are saying, but its a bit confusing ask this here since it doesn't involve this module.

For the specific lines you mention: tokenId is a uint256 as defined by erc721, and the x and y arguments need to be anything that will be able to contain integers from 0 to 512, and uint256 works for that. All of the arguments are going to be converted to strings before being concatenated into the sql statement, so what ever type reduces gas while still working is probably the right choice. Note smaller doesn't necessarily mean cheaper.
With all that said, the Canvas Game is a work in progress so it might be premature to be finding nits.

@wbt
Copy link
Author

wbt commented Jul 28, 2022

There are three different type systems involved in a DApp using Tableland: Solidity's, Tableland's, and JavaScript's; there's often a fourth related to application logic. When developing a DApp, it's important to make sure these types are mapping onto one another correctly. For example, I have a value which is an enum in Solidity, a string in JavaScript, a keyof union type in TypeScript, and an int in Tableland. This requires more explicit mapping at interfaces, but the implicit mappings tend to be the ones that cause more trouble so it's important to pay close attention to them.

The only example I found of using Tableland in a smart contract to see how the Tableland types map onto Solidity was the Canvas Game, which isn't even discoverable on the main branch of the examples repo. That is was what was available to go on, and it led me to an incorrect belief that Tableland's int mapped to Solidity's uint256. I don't think this was an unreasonable conclusion from the limited information available, so others may have been similarly led astray and might be led astray in the future if there is no change.

Because a uint256 in Solidity definitely maps onto BigNumber in Javascript, but TableLand returns a JavaScript number instead, there was an inconsistency and broken type system. That is what led to the question about whether or not large numbers are correctly handled, and some experimentation to reveal that just using Tableland as documented will lead to some unexpected behavior once the following-current-docs solution hits sufficiently large numbers. Debugging could be hard because of the conditional/intermittent nature of the resulting bug (occurring only when certain values, which might not be visible to a user, exceeds a certain threshold, which is not obvious especially to folks who don't have a lot of programming experience).

I think the main proposal here is to either use BigNumbers of some sort in SDK read responses, and/or be WAY more obvious and clear in the documentation that Tableland int fields should NOT be used to store values which can't be safely represented as a JavaScript number, even if the INSERT/UPDATE action which adds the number is being called from the EVN with types that can handle such large numbers without issue.

@joewagner
Copy link
Contributor

@wbt The mapping of the different type systems to each other is certainly a concern for every Dapp, or app, using Javascript and a SQL Database of any kind. In general if the app wants to support Javascript integers above MAX_SAFE_INTEGER storing the data as a string should work.
If this isn't the case for you, can you provide a PR that implements your suggestion or a running app with an issue?

@wbt
Copy link
Author

wbt commented Jul 29, 2022

In general if the app wants to support Javascript integers above MAX_SAFE_INTEGER storing the data as a string should work.

Yes, that is generally true, especially since Tableland read operations from the EVM aren't supported (!).
However, making the choice to store the data as a string is a decision that should be made pretty early in building a DApp, and the current documentation around integer types does NOT at all make this clear. It should.

@carsonfarmer
Copy link
Member

Thanks for raising this ticket. It has kicked off a good deal of internal discussion (in addition to the discussions here and on Discord). The action item here is to update documentation and recommendations, and make this clearer to those digging into our docs.We will likely pull in some of the content from this discussion here as well. I'm leaving this open to ensure we circle back to make sure the questions have been addressed.

cc @dtbuchholz for your docs updates.

@joewagner
Copy link
Contributor

@carsonfarmer
Copy link
Member

I'd like to wait to close it until we're satisfied that the docs updates have indeed been made.

@dtbuchholz
Copy link
Contributor

@carsonfarmer lmk if anythings missing https://docs.tableland.xyz/solidity-to-sql-types

@joewagner
Copy link
Contributor

lmk if anythings missing https://docs.tableland.xyz/solidity-to-sql-types

@dtbuchholz this looks good to me. @carsonfarmer should we solicit feedback from users here?

@carsonfarmer
Copy link
Member

No I think we can close this and get feedback on an ongoing basis.

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

No branches or pull requests

4 participants