-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Block.js #300
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
Conversation
|
ping @weilu |
|
the API LGTM Is this going to be available before 3.0.0? |
|
I'm trying to evaluate the API by using it: weilu/cb-blockr@c522315 @dcousens I'll have something for you by the end of this week. Sorry for the delay |
|
No worries, look forward to your feedback. webbtc.com provide a hex end point if you're looking for one. |
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.
Is this function necessary? When I parse dates it's normally for display reasons, where local timezone would be more useful than UTC.
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.
Can this problem be solved with documentation instead of exposing a function may or may not be useful to users?
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 don't mind if we have getUTCDate or getLocalDate, but the important point is that the block.timestamp is UTC seconds, not your typical JS Date.
Personally, I find the UTC date useful because it outputs in your timezone anyway.
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.
My point is that this is a utility function that I expect to write myself as an app dev. I don't think it's relevant to block parsing
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, this is a Javascript interface to a Block, so it seemed pertinent to provide a Javascript Date which deals with the .timestamp being stored in seconds correctly.
I'm open to other interpretations though.
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.
Still conflicted on this. But I think I'm going to err on the side of minimizing possible user error by providing it, and if we don't see usage we can deprecate it later?
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.
ok
|
Looks good overall. When constructing a block by hand (as opposed to using the deserialization function), it wasn't immediately obvious that |
|
Just a note: network.js |
|
I think that is a good point about
What is the relevance here? |
|
Those two will need to be renamed if we were to decide to go ahead with the |
|
Right. Not sure on that change. |
|
@dcousens if you don't want to make any amendments go ahead merge it as-is. It's a small enough API can iterate on |
|
+1 from me. This is an awesome feature to add to the library! |
|
Rebased on HEAD |
Resolves #279 .
The API right now is very simple, but provides the basic building blocks.
I'm not sure whether we should put merkle tree hashing in here or some where else.
Mostly this abstraction was created to be able to parse blocks, further functionality can be added when necessary?
@weilu thoughts?