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

varstruct - WIP #554

Closed
wants to merge 1 commit into from
Closed

Conversation

fanatid
Copy link
Member

@fanatid fanatid commented Feb 27, 2016

varstruct for encode/decode blocks/txs?

@dcousens
Copy link
Contributor

Will review soon :)

@fanatid
Copy link
Member Author

fanatid commented Mar 4, 2016

@dcousens please check it, I want to know you opinion about this.
I fixed varstruct and now fromBuffer works fine. toBuffer doesn't work because txOut can have valueBuffer instead value (I don't know how handle it, may be create custom codec...).

@mappum since same structures are used in bitcoin-protocol maybe make sense create package bitcoin-struct-tx-block or two bitcoin-tx-struct and bitcoin-block-struct?

@mappum
Copy link

mappum commented Mar 4, 2016

Maybe we should just make one bitcoin-struct package with all the structs in it?

@fanatid
Copy link
Member Author

fanatid commented Mar 4, 2016

Sorry, what do you mean under all ? tx and block ?

@fanatid
Copy link
Member Author

fanatid commented Mar 9, 2016

ping

@mappum
Copy link

mappum commented Mar 9, 2016

I was thinking there might be other structs we might want to include, such as merkleblocks and full blocks (although those are not used by bitcoinjs).

@fanatid
Copy link
Member Author

fanatid commented Apr 1, 2016

Looks like not actual right now, hope varstruct will be used in #508

@fanatid fanatid closed this Apr 1, 2016
@dcousens
Copy link
Contributor

dcousens commented Apr 1, 2016

@fanatid this looks really good 👍

@dcousens dcousens assigned dcousens and unassigned fanatid Apr 1, 2016
@dcousens
Copy link
Contributor

dcousens commented Apr 1, 2016

bitcoin-structs might work, assuming we just put all the basic POD types in there.

Transaction, Block? Any others?

@fanatid
Copy link
Member Author

fanatid commented Apr 1, 2016

or move messages from bitcoin-protocol to bitcoin-protocol-messages and use tx, block from there
https://github.com/mappum/bitcoin-protocol/blob/10d67127c89ed1928094be84375f70dfef0a43b3/src/messages.js

@dcousens dcousens reopened this Apr 1, 2016
@dcousens
Copy link
Contributor

dcousens commented Apr 1, 2016

@fanatid looks good to me.
Though that would make #440 either impossible, or something we'd need to do in lockstep with @mappum's bitcoin-protocol.

@dcousens
Copy link
Contributor

dcousens commented Apr 1, 2016

Maybe @mappum we could move bitcoin-protocol to the bitcoinjs-org, you'd still be lead contributor, but then it would be clear that the repository is linked (and hence in lockstep)?

@fanatid
Copy link
Member Author

fanatid commented Apr 1, 2016

@dcousens if you wanted to move bitcoin-protocol you probably should do same with bitcoin-net

@dcousens
Copy link
Contributor

dcousens commented Apr 1, 2016

Well, that is up to @mappum

@dcousens
Copy link
Contributor

dcousens commented Jul 11, 2016

I started playing with this @fanatid, and ran into issues with valueBuffer edge cases.
I'd have to make a custom encode/decode that handled both cases (Buffer and Number).

Handling this that way is fine, and welcome, however, it would be a breaking change.

@dcousens dcousens changed the title [WIP] varstruct varstruct - WIP Jul 11, 2016
@dcousens dcousens added this to the 3.0.0 milestone Jul 11, 2016
@dcousens
Copy link
Contributor

Closing for now

@dcousens dcousens closed this Dec 17, 2016
@dcousens
Copy link
Contributor

Still covered by #513

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

Successfully merging this pull request may close these issues.

3 participants