-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Refactor version anchor #659
Conversation
e84c498
to
61ed686
Compare
src/Configuration.ts
Outdated
), | ||
function validatePoetVersion(poetVersion: number) { | ||
assert( | ||
Number.isInteger(poetVersion) && 0 <= poetVersion && poetVersion < 4294967296, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be 65536? (256 ** 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played around with writeUInt16BE and writeUInt16LE and I found that the maximums numbers are:
writeUInt16LE 65535 (2 ^ 16)
writeUInt16BE 4294967296 (2 ^ 32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. How can endianness affect the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I received this error when I'm out the range:
writeUInt16BE:
RangeError [ERR_OUT_OF_RANGE]: The value of "value" is out of range. It must be >= 0 and <= 4294967295. Received 4294967296
writeUInt16LE:
RangeError [ERR_OUT_OF_RANGE]: The value of "value" is out of range. It must be >= 0 and <= 65535. Received -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised an issue about this to NodeJS and they already fixed it! 😄
So we should update our code accordingly.
Number.isInteger(poetVersion) && 0 <= poetVersion && poetVersion <= 0xFFFF
src/BlockchainWriter/Bitcoin.ts
Outdated
|
||
try { | ||
buffer.writeUInt16BE(version, 0) | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we replacing any error thrown by writeUInt16BE
with IllegalVersionLength
here?
Is an incorrect version length the only thing that could go bad?
Does an incorrect version length actually throw an error?
src/BlockchainReader/Bitcoin.ts
Outdated
@@ -78,7 +78,8 @@ export const isCorrectBufferLength = (buffer: Buffer) => buffer.byteLength >= an | |||
|
|||
export const bufferToPoetAnchor = (buffer: Buffer): PoetAnchor => { | |||
const prefix = buffer.slice(0, 4).toString() | |||
const version = Array.from(buffer.slice(4, 6)) | |||
// const version = buffer.slice(4, 6).readUInt16BE(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
8b5e90b
to
1fc51dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 🚀
🎉 This PR is included in version 2.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
PR Process - PR Review Checklist
Release
Semantic release is enabled for this repository. Make sure you follow the right commit message convention.
We're using semantic-release's default — Angular Commit Message Conventions.
Description of Changes
Fix #491
This PR introduces: