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

Fixed the type for lamport #1286

Closed
wants to merge 1 commit into from

Conversation

kunxl-gg
Copy link

@kunxl-gg kunxl-gg commented May 3, 2023

Fixes #1116

  • As far as I understood we need to change the type of Lamports to String everywhere. If I understand this right, I can make further PRs, to help in the rewrite of the code.

@mergify mergify bot added the community label May 3, 2023
@mergify mergify bot requested a review from a team May 3, 2023 13:03
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I'm looking at the RPC responses in the docs and they appear to be u64 everywhere.

> curl https://api.mainnet-beta.solana.com -X POST -H "Content-Type: application/json" -d '
  {"jsonrpc":"2.0","id":1, "method":"getRecentBlockhash"}
'

{"jsonrpc":"2.0","result":{"context":{"apiVersion":"1.13.7","slot":192403900},"value":{"blockhash":"D59Fvi8JpnEMBdbeRX2PbfZreqrd2R5kxn3FAYkfWHNf","feeCalculator":{"lamportsPerSignature":5000}}},"id":1}

While it would be great for these to be actual bigint values, you can't serialize bigint as JSON. Serializing the integer as a string over the wire is a good strategy, doing so as you've done in this PR would break old clients.

@kunxl-gg
Copy link
Author

kunxl-gg commented May 6, 2023

I'm looking at the RPC responses in the docs and they appear to be u64 everywhere.

> curl https://api.mainnet-beta.solana.com -X POST -H "Content-Type: application/json" -d '
  {"jsonrpc":"2.0","id":1, "method":"getRecentBlockhash"}
'

{"jsonrpc":"2.0","result":{"context":{"apiVersion":"1.13.7","slot":192403900},"value":{"blockhash":"D59Fvi8JpnEMBdbeRX2PbfZreqrd2R5kxn3FAYkfWHNf","feeCalculator":{"lamportsPerSignature":5000}}},"id":1}

While it would be great for these to be actual bigint values, you can't serialize bigint as JSON. Serializing the integer as a string over the wire is a good strategy, doing so as you've done in this PR would break old clients.

So shall I just make a js function throw an error if the value of lamports exceeds the limit

@steveluscher
Copy link
Collaborator

This is actually how the new web3.js rewrite will behave (link). I think it's too late at this point to change the behaviour of the existing library, because it would break existing apps.

I'm sorry that you picked this issue up with the ‘good first issue’ tag. I think it might be more complicated than a quick fix.

@kunxl-gg
Copy link
Author

kunxl-gg commented May 7, 2023

Oh my gosh. I am so sorry for bugging you. I was just trying to play around with this library and solve some simple issues to get involved in the community. I am closing the PR right away.

@kunxl-gg kunxl-gg closed this May 7, 2023
@steveluscher
Copy link
Collaborator

No need to be sorry! Thank you for contributing to Solana!

@github-actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON RPC API using (u)int64 whereas JS can only handle int53
2 participants