-
Notifications
You must be signed in to change notification settings - Fork 23
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
Blob #71
Blob #71
Conversation
throw new Error('Invalid blob') | ||
} | ||
|
||
static fromString(value: string) { |
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.
This needs to handle the invalid base64 padding nodeos outputs
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 think I have tests ensuring the invalid stuff works here: 1cd6b9b
src/chain/blob.ts
Outdated
} | ||
|
||
static fromString(value: string) { | ||
return new this(new Uint8Array(Buffer.from(value, 'base64'))) |
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.
Didn't catch this first time around but you cannot use Buffer in core, it's not available outside of node.js. You'll have to use atob and btoa (see https://developer.mozilla.org/en-US/docs/Glossary/Base64)
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. I originally swapped to a buffer because VSCode was complaining atob was deprecated 😅
New snag... atob
and btoa
aren't available in nodejs 14, so our tests against that version are failing. I added back some checks to see if Buffer
exists, and if so, use it.
src/chain/blob.ts
Outdated
} | ||
|
||
get base64String(): string { | ||
return Buffer.from(this.array).toString('base64') |
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.
Same here, no Buffer
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.
LGTM, just a nitpick
src/chain/blob.ts
Outdated
|
||
/** | ||
* Create a new Bytes instance. | ||
* @note Make sure to take a [[copy]] before mutating the bytes as the underlying source is not copied here. |
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.
Docstring says Bytes and references non existent method
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.
Bytes
is where I copied the class structure from instead of writing it from scratch... my sloppy copy/pasta. Whoops 😅
No description provided.