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

Clean up the use of dcodeIO/bytebuffer.js #1932

Closed
faustbrian opened this Issue Jan 3, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@faustbrian
Copy link
Collaborator

faustbrian commented Jan 3, 2019

Right now the use of the ByteBuffer lib is pretty messy and can be cleaned up quite a bit.

Offsets are specified manually like seen here https://github.com/ArkEcosystem/core/blob/develop/packages/crypto/src/models/transaction.ts#L181. A call to readInt8 will automatically move the cursor forward 1 byte which means we continue reading at byte 2 so there is no need to manually specify readInt8(2), readInt8() will be sufficient.

We are also using native string methods to read things like public keys for which we instead could use ByteBuffer.readString to guarantee consistent behaviour based on the offsets per type.

Offsets are automatically set by the library as can be seen in the docs at https://github.com/dcodeIO/bytebuffer.js/wiki/API which means there is no need for us to manually specify any offsets, which reduces the possibility of mistakes.

@JeremiGendron

This comment has been minimized.

Copy link
Contributor

JeremiGendron commented Jan 9, 2019

from my tests:

buf.readInt8() // returns version OK
buf.readInt8() // returns -1 for network (not OK)

this is with buf.clone() for the tested buffer

It seems to work OK when setting an offset (amount) before reading

clone.offset = 1
clone.readInt8() // version 1
clone.readInt8() // network 30
clone.readInt8() // type 0

@supaiku0 supaiku0 self-assigned this Jan 9, 2019

@faustbrian faustbrian self-assigned this Jan 9, 2019

@supaiku0

This comment has been minimized.

Copy link
Contributor

supaiku0 commented Jan 9, 2019

@JeremiGendron That is because offset 0 contains 0xff which needs to be accounted for. I'm refactoring part of the crypto models and this issue is on my list.

@JeremiGendron

This comment has been minimized.

Copy link
Contributor

JeremiGendron commented Jan 9, 2019

clone.readBytes(33).toString("hex") ===  transaction.senderPublicKey = hexString.substring(16, 16 + 33 * 2);

stopping now...

@JeremiGendron

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment