Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Fix insert a node in the middle leaf #14

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

jbaylina
Copy link
Contributor

@jbaylina jbaylina commented Nov 1, 2016

This PR fix the test designed here:

ethereum/tests#125

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.44% when pulling e896de2 on jbaylina:fix-insert-middle-leaf into 828febf on ethereumjs:master.

@jbaylina jbaylina changed the title Fix insert a ne node in the middle leaf Fix insert a node in the middle leaf Nov 1, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.44% when pulling 85d904d on jbaylina:fix-insert-middle-leaf into 828febf on ethereumjs:master.

@jbaylina jbaylina mentioned this pull request Nov 2, 2016
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This looks good to me. The added test branch-value-update in the official test file https://github.com/ethereum/tests/blob/develop/TrieTests/trietest.json is actually not triggered yet, due to the static nature (sub module include) of the ethereumjs-testing module.

I tested this manually though, tests are passing, so I will approve. Will do a subsequent PR with removing ethereumjs-testing dependency and adding the three test files locally for being more flexible to update here and not have the heavy burden of downloading the whole test suite every time.


// Check if the last node is a leaf and the key matches to this
var matchLeaf = false
if (lastNode.type === 'leaf') {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, if this is the most efficient way to do the key length comparison, won't take this as a blocker though.

@holgerd77 holgerd77 merged commit bb2c366 into ethereumjs:master Nov 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants