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

JS v3: Native bigint support for select fields #72

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented Jan 10, 2024

Currently JS types every numeric field as number | bigint. This isn't very useful, since you can't operate directly against this union type.

This PR allows the generated types to use either number or bigint, depending mostly on the x-algorand-format: uint64 tag on fields. Previously this tag was only used to decided whether the Java SDK should use Long or BigInteger for a field.

However, there are two reasons why that approach can only go so far:

  • JS numbers are a unique size, so it's necessary to upgrade to bigint in cases where Java's Long would be sufficient.
  • The existing usage of the x-algorand-format: uint64 tag is lacking/inconsistent in some places.

To address these issues, I added the ability to manually override types by adding to one of the property files.

For consistency, I made the following quantities always have type bigint, even if they weren't declared with x-algorand-format: uint64 in the REST spec:

  • Algo balances/fees/rewards
  • Asset balances
  • Application and asset IDs
  • Round numbers and ranges of rounds (like upgradeDelay)

These changes are present in this JS SDK PR: algorand/js-algorand-sdk#852

@jasonpaulos jasonpaulos marked this pull request as ready for review February 15, 2024 15:32
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

I guess I'm wondering why should support all of these options in property files, what's the use case for clients needing the ability to override?

# otherwise operated on with a bigint, it should be a bigint for ease of use
# * Other quantities outside of the above and which will never exceed the maximum
# safe javascript integer should be number
type_override_Account_amount=bigint
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question, why should we leave this as configurable at all? While we are putting sane defaults here, what are the cases where we should support the client overriding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First I should explain what's happening in this file. It's really just a list of key-value definitions.

In the template code, specifically here:

#set( $d = "$" )
#set( $e = "!" )
#set( $type_override_variable = "${d}${e}propFile.type_override_${className}_#paramName(${param})" )
#set( $type_override = "#evaluate($type_override_variable)" )

we look at propFile.type_override_X_Y, where X is the generated class name and Y is the field in that class. If there's a value associated with that key from the prop file, we will use the type override that it declares.

So it's less about making each field configurable, and more about configuring the list of fields and their overrides.

I chose to put the overrides in this config file for a few reasons:

  1. Different fields needed to be overriden for algod and indexer
  2. It's much easier to encode this information in a file like this instead of placing a massive if/elseif/.../else statement in the template code

@jasonpaulos jasonpaulos merged commit b9042f4 into js-v3 Feb 16, 2024
2 checks passed
@jasonpaulos jasonpaulos deleted the js-native-bigint branch February 16, 2024 20:37
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.

3 participants