Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Improve type definitions for decode #78

Closed
s1na opened this issue Aug 1, 2019 · 1 comment · Fixed by #109
Closed

Improve type definitions for decode #78

s1na opened this issue Aug 1, 2019 · 1 comment · Fixed by #109

Comments

@s1na
Copy link
Contributor

s1na commented Aug 1, 2019

Thinking abstractly about decode I'd expect a function signature such as:

export function decode(input: Buffer): NestedBufferList

where NestedBufferList could be any nested composition of buffers and arrays such as Buffer, or Buffer[], or [Buffer, Buffer[]], etc. (haven't tried expressing this in TS).

Currently the decode method has the following definitions:

rlp/src/index.ts

Lines 60 to 63 in f06a96f

export function decode(input: Buffer, stream?: boolean): Buffer
export function decode(input: Buffer[], stream?: boolean): Buffer[]
export function decode(input: Input, stream?: boolean): Buffer[] | Buffer | Decoded
export function decode(input: Input, stream: boolean = false): Buffer[] | Buffer | Decoded {

  1. First definition takes Buffer and returns a Buffer. Not sure why, it could very well be that the decoding is a NestedBufferList. This was also the problem stated in Encoding and Decoding RLP lists when part of a buffer #77
  2. Second def. takes a Buffer[]. Is this to support decoding multiple serialized items simultaneously? the code doesn't seem to do that. I suggest we remove this
  3. The last two take Input as argument. I imagine this is for backwards-compatibility. I suggest we tighten the API and only accept a Buffer

On the stream flag: we could keep it, or add a new function decodeStreaming.

@wemeetagain
Copy link
Contributor

wemeetagain commented Sep 19, 2019

An additional wrinkle, the readme says :
rlp.decode(encoded, [skipRemainderCheck=false]) - Decodes an RLP encoded Buffer, Array or String and returns a Buffer or an Array of Buffers. If skipRemainderCheck is enabled, rlp will just decode the first rlp sequence in the buffer. By default, it would throw an error if there are more bytes in Buffer than used by rlp sequence.

It looks like the skipRemainderCheck logic is gone.

EDIT:
the skipRemainderCheck param isn't needed anymore because the stream param does the same thing and more.

@ryanio ryanio linked a pull request Jan 3, 2022 that will close this issue
@ryanio ryanio linked a pull request Jan 9, 2022 that will close this issue
ryanio added a commit that referenced this issue Jan 10, 2022
* update karma-typescript to use fix on master not yet released: monounity/karma-typescript#502
* update readme
* bin/rlp: use capital RLP, destructure bytesToHex
* destructure ternary operator for easier readability
* tidy, touch ups, flatten return statements when possible, move utils to own file
* destructure utils usage, move numberToBytes to test utils
* eslint updates
* rename baToJSON to arrToJSON
* add new test coverage from @jochem-brouwer, add es2020 property to eslint, renamings
* update package.json version to 3.0.0, add changelog entries
* remove separate browser build since tsconfigs are the same. tested with browserify and was able to RLP encode in the browser from dist/index.js
* fix karma-typescript
* ci:
  - use npm 7 for github url integrity support (for karma-typescript unreleased workaround)
  - run coverage inside node v16 workflow so a duplicative job is not needed
* ci: more clarity on reason for added npm v7 and when it can be removed
* combine to one file to improve esm, deno support
* update changelog
* Add NestedUint8Array type (fixes issue #78)
* tidy readme
* changelog: add cli PR
* nit: keys by alphabetical order
* replace node 13 with 17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants