Skip to content
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

Update merkle-patricia-tree to v4, move account trie-related methods to StateManager #787

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jun 19, 2020

This PR:

  • Updates merkle-patricia-tree to v4
  • Moves account trie-related methods to StateManager: getCode, setCode, getStorage, setStorage
    • this was a suggestion from @s1na since he believes the vm was the only consumer of these methods
  • Removes various unneeded promisifys

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #787 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
- Coverage   84.40%   84.34%   -0.07%     
==========================================
  Files          19       17       -2     
  Lines        1257     1233      -24     
  Branches      249      246       -3     
==========================================
- Hits         1061     1040      -21     
+ Misses        126      125       -1     
+ Partials       70       68       -2     
Flag Coverage Δ
#account 92.85% <100.00%> (-1.74%) ⬇️
#block 79.97% <100.00%> (+0.12%) ⬆️
#blockchain 84.50% <ø> (ø)
#common 93.60% <ø> (-0.19%) ⬇️
#ethash 86.30% <ø> (+0.89%) ⬆️
#tx 94.16% <ø> (ø)
Impacted Files Coverage Δ
packages/account/src/index.ts 92.85% <100.00%> (-1.74%) ⬇️
packages/block/src/block.ts 71.00% <100.00%> (+0.52%) ⬆️
packages/common/src/genesisStates/index.ts 100.00% <0.00%> (ø)
packages/common/src/hardforks/index.ts
packages/common/src/chains/index.ts
packages/ethash/src/util.ts 97.22% <0.00%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6beae4...9cfc6ab. Read the comment docs.

@ryanio ryanio changed the title Update merkle-patricia-tree to v4, update account trie-related methods to async Update merkle-patricia-tree to v4, move account trie-related methods to StateManager Jun 19, 2020
@lgtm-com

This comment has been minimized.

@holgerd77
Copy link
Member

Just had a first look, phew, nice PR! 😄

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, phew, great work! 👍 😄 🎉

@@ -141,7 +144,7 @@ export default class DefaultStateManager implements StateManager {
async _lookupStorageTrie(address: Buffer): Promise<Trie> {
// from state trie
const account = await this.getAccount(address)
const storageTrie = this._trie.copy()
const storageTrie = this._trie.copy(false)
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this one was important to maintain the original behavior.

@holgerd77 holgerd77 merged commit fe4ded6 into master Jun 26, 2020
@holgerd77 holgerd77 deleted the mpt-v4 branch June 26, 2020 11:23
Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

Late review — I see the code was merged while I was reviewing. Good work, though. I like how the new syntax looks like and the test suite improvements.

@@ -48,7 +48,6 @@
"@types/bn.js": "^4.11.6",
"@types/node": "^11.13.4",
"@types/tape": "^4.13.0",
"merkle-patricia-tree": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -43,7 +43,7 @@
"@ethereumjs/tx": "^2.1.2",
"@types/bn.js": "^4.11.6",
"ethereumjs-util": "^7.0.2",
"merkle-patricia-tree": "^2.3.2"
"merkle-patricia-tree": "^4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
})
})
await this.txTrie.put(rlp.encode(txIndex), tx.serialize())
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's much better :)

account.balance.toString('hex') === '' &&
account.codeHash.toString('hex') === KECCAK256_NULL_S
)
return account.isEmpty()
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants