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

docs: add a section for numbers #1768

Merged
merged 14 commits into from
Feb 23, 2024
Merged

docs: add a section for numbers #1768

merged 14 commits into from
Feb 23, 2024

Conversation

Dhaiwat10
Copy link
Member

Closes #1555

@Dhaiwat10 Dhaiwat10 added the docs Requests pertinent to documentation label Feb 18, 2024
@Dhaiwat10 Dhaiwat10 self-assigned this Feb 18, 2024
@Dhaiwat10 Dhaiwat10 enabled auto-merge (squash) February 19, 2024 18:05
arboleya
arboleya previously approved these changes Feb 19, 2024
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Do we think it's a good idea to introduce a BigInt example also, as those working with ethers made need assistance with conversions?

Additionally a sub paragraph on handling and differentiating big numbers I think would be beneficial. As you only need bn for u64.

apps/docs/src/guide/types/numbers.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel Bate <djbate23@gmail.com>
@Dhaiwat10
Copy link
Member Author

Do we think it's a good idea to introduce a BigInt example also, as those working with ethers made need assistance with conversions?

@danielbate as in, how does one convert a BigInt into a BN object? Or rather, how does one directly use an ethers-compatible BigInt with our SDK?

Additionally a sub paragraph on handling and differentiating big numbers I think would be beneficial. As you only need bn for u64.

Interesting. Did not know that. So, for the smaller types - we can directly pass in JavaScript numbers?

@danielbate
Copy link
Contributor

as in, how does one convert a BigInt into a BN object? Or rather, how does one directly use an ethers-compatible BigInt with our SDK?

This may not even be possible, but BigInt and BN conversions so it can be used with both our SDK and ethers. If it's not possible, feel free to disregard.

Interesting. Did not know that. So, for the smaller types - we can directly pass in JavaScript numbers?

Yes exactly that, so if we look at the encode function for a NumberCoder which would be used when the ABI references a u8, u16 or u32:

encode(value: number | string): Uint8Array {

The value to be encoded is expecting a number rather than a bn, so we can safely pass a JS compatible number.

danielbate
danielbate previously approved these changes Feb 22, 2024
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Nice one Dhai 💪🏻 Just one thought, may be irrelevant so approving.

apps/docs/src/guide/types/numbers.md Show resolved Hide resolved
apps/docs/src/guide/types/numbers.md Show resolved Hide resolved
@Dhaiwat10
Copy link
Member Author

@Torres-ssf @danielbate thanks for the heads-up, investigating u256 support

@Dhaiwat10 Dhaiwat10 marked this pull request as draft February 23, 2024 03:20
auto-merge was automatically disabled February 23, 2024 03:20

Pull request was converted to draft

@Dhaiwat10
Copy link
Member Author

@danielbate @Torres-ssf you were right lads, we dont support u256. I've added a notice for it in the docs.

Should I also create an issue to add support for this type? @arboleya

@Dhaiwat10 Dhaiwat10 marked this pull request as ready for review February 23, 2024 03:31
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
80.01%(+0%) 71.05%(+0%) 78%(+0%) 80.07%(+0%)
Changed Files:

Coverage values did not change👌.

@arboleya
Copy link
Member

Should I also create an issue to add support for this type? @arboleya

Hmm, good catch. Let's also check if the Rust SDK has it.

There may be some reason it has never been added; it's worth investigating.

But yes, unless there's some strong reason behind it, feel free to create an issue. 👍

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

@Dhaiwat10 Dhaiwat10 dismissed Torres-ssf’s stale review February 23, 2024 17:35

Removed all mentions of u256

@Dhaiwat10 Dhaiwat10 merged commit 618c0c2 into master Feb 23, 2024
13 checks passed
@Dhaiwat10 Dhaiwat10 deleted the dp/numbers-docs branch February 23, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Requests pertinent to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No section around numbers
4 participants