Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

provide uncle size where available in RPC #4713

Merged
merged 1 commit into from Mar 2, 2017
Merged

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Mar 1, 2017

cc @arkpar

Also adds a test for the Block and RichBlock types that size: None goes to null as expected.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 1, 2017
@tomusdrw
Copy link
Collaborator

tomusdrw commented Mar 1, 2017

I think it should actually be 0, if we want to be aligned with geth.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 2, 2017
@gavofyork
Copy link
Contributor

geth is arbitrary and, in any sensible reading, flat-out wrong. null is the most correct answer here and we'll stick with it.

@gavofyork gavofyork merged commit 31302e0 into master Mar 2, 2017
@gavofyork gavofyork deleted the rpc-uncle-size branch March 2, 2017 11:29
@tomusdrw
Copy link
Collaborator

tomusdrw commented Mar 2, 2017

@gavofyork @rob do I guess the change here should be reverted since it will return non-null values if the full.block is available for an uncle.

@rphmeier
Copy link
Contributor Author

rphmeier commented Mar 2, 2017

@tomusdrw Not sure if I follow, that's the whole point of the PR. The RPC isn't that well designed, so it is supposed to return the same result as eth_getBlockBy*, which includes a size. The size isn't always available, but we should return it when it is.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Mar 2, 2017

So I guess it should consistently return null for uncles. Don't see a point in returning non-reliable size in any case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants