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

Fix up the transaction JSON serialisation for RPC. #3633

Merged
merged 10 commits into from
Dec 7, 2016
Merged

Conversation

gavofyork
Copy link
Contributor

No description provided.

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Nov 27, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.927% when pulling 0cf8db5 on fix-tx-rpc into a7037f8 on master.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Nov 28, 2016

Shouldn't standardV and v be encoded as QUANTITY alike everything else?

Looks good otherwise apart from the failing test.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 63c2825 on fix-tx-rpc into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 63c2825 on fix-tx-rpc into ** on master**.

fn to_big_endian(&self, bytes: &mut[u8]);
/// Convert to a non-zero-prefixed hex representation prefixed by `0x`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not prefixed by 0x

@@ -685,6 +687,17 @@ macro_rules! construct_uint {
}

#[inline]
fn to_hex(&self) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if LowerHex trait implementation would be more idiomatic, @rphmeier?

Copy link
Contributor Author

@gavofyork gavofyork Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, but we have to_hex implemented for at least one other type used here and since this is bug fix PR i'd prefer to leave that for a second refactoring PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this via LowerHex is indeed more idiomatic and should save allocation costs while formatting it as part of any other message (this is actually something to be mildly concerned about -- @arkpar and I found out recently that enabling any logging will cause the format arguments for all logs at that level, even with differing targets, to be evaluated)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d58905a on fix-tx-rpc into ** on master**.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 6, 2016
@gavofyork gavofyork merged commit 1f0a02b into master Dec 7, 2016
@gavofyork gavofyork deleted the fix-tx-rpc branch December 7, 2016 13:34
@tomusdrw tomusdrw restored the fix-tx-rpc branch January 3, 2017 08:23
@tomusdrw tomusdrw deleted the fix-tx-rpc branch January 3, 2017 08:24
@tomusdrw tomusdrw mentioned this pull request Jan 3, 2017
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. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants