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

Transaction Metadata overview: Correct ranges of 64bit integers #1177

Closed
wants to merge 3 commits into from
Closed

Conversation

bjornkihlberg
Copy link

@bjornkihlberg bjornkihlberg commented Dec 7, 2023

Checklist

  • I have read the How to Contribute.
  • I have run yarn build after adding my changes without getting any errors.
    No, I'm only changing the readme.

Updating documentation or Bugfix

Correct ranges of 64bit signed and unsigned integers.

I'm assuming it should be a 64bit integer, in which case the ranges are currently incorrect. Here's a stackoverflow thread with an answer with 87 upvotes strengthening my argument if you don't believe me.

Correct ranges of 64bit signed and unsigned integers.
@bjornkihlberg bjornkihlberg changed the title Update overview.md with Correct ranges of 64bit signed and unsigned integers Update overview.md with correct ranges of 64bit signed and unsigned integers Dec 7, 2023
@rphair rphair changed the title Update overview.md with correct ranges of 64bit signed and unsigned integers Transaction Metadata overview: Correct ranges of 64bit signed and unsigned integers Dec 8, 2023
@rphair rphair changed the title Transaction Metadata overview: Correct ranges of 64bit signed and unsigned integers Transaction Metadata overview: Correct ranges of 64bit integers Dec 8, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

The range after Integers in the range is obviously impossible with a 64-bit value... but I think we need a definitive reference or schema saying whether this value is signed or unsigned.

That section, even after your change, implies the integer metadata is a signed value... while the earlier text "between 0 and ..." for your first change suggests that it is unsigned. This was also a problem with the original text and therefore also indicates a need to reference the source material in this PR and probably also the document itself.

@rphair rphair added the documentation Improvements or additions to documentation label Dec 8, 2023
@os11k
Copy link
Collaborator

os11k commented Dec 27, 2023

Thank you for your PR! I would like to add, even if you "only changing the readme" it is mandatory to run yarn build, that why it is in a checklist. Stackoverflow is nice source of information, but more hard evidences would be necessary, imho.

@bjornkihlberg bjornkihlberg closed this by deleting the head repository Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants