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

structured rlp encoding in journaldb #8047

Merged
merged 2 commits into from
Mar 15, 2018
Merged

structured rlp encoding in journaldb #8047

merged 2 commits into from
Mar 15, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Mar 4, 2018

As a part of #8033, I'm trying to get rid of all usages of Rlp. This pr get rids of unsafe usages of Rlp in journaldb. To reduce the number of calls to expect() I introduced structured encoding in the module.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 4, 2018
@5chdn 5chdn added this to the 1.11 milestone Mar 5, 2018
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones merged commit 21cb085 into master Mar 15, 2018
}).expect("Low-level database error.") {
let rlp = Rlp::new(&rlp_data);
let inserts: Vec<H256> = rlp.list_at(1);
//let mut index = 0usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed.

}

impl<'a> DatabaseValueView<'a> {
pub fn new(data: &'a [u8]) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to rename this to something like from_rlp?

@andresilva
Copy link
Contributor

Guess I was too late but I had a pending review for this. Just minor stuff though.

@ascjones
Copy link
Contributor

Too quick on the draw - I can make those changes if okay with @debris

@debris debris deleted the journaldb_rlp branch March 15, 2018 12:39
General-Beck added a commit that referenced this pull request Mar 19, 2018
@ascjones ascjones restored the journaldb_rlp branch March 19, 2018 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants