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

refactor(trie): mark db as protected to avoid leaky properties #2221

Merged
merged 35 commits into from
Aug 24, 2022

Conversation

faustbrian
Copy link
Contributor

Leaving as draft until #2215 is merged

The db property shouldn't be accessible outside of the trie instance because we wrap it so opts.db and this.db aren't equal and in general everything that can be non-public should be non-public by default. Always start with private and only work up if necessary.

holgerd77 and others added 30 commits August 23, 2022 12:32
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #2221 (a484195) into master (fffe4ab) will increase coverage by 0.17%.
The diff coverage is 97.36%.

Impacted file tree graph

Flag Coverage Δ
block 92.98% <ø> (ø)
blockchain 88.88% <ø> (?)
client 87.00% <ø> (ø)
common 59.55% <ø> (ø)
devp2p 92.25% <ø> (-0.09%) ⬇️
ethash ∅ <ø> (∅)
evm 79.07% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 88.17% <100.00%> (+0.02%) ⬆️
trie 89.89% <96.96%> (-0.06%) ⬇️
tx 97.98% <ø> (ø)
util 92.33% <ø> (ø)
vm 85.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Base automatically changed from trie-checkpoint-option to master August 24, 2022 09:46
@faustbrian
Copy link
Contributor Author

@holgerd77 conflicts resolved

@faustbrian faustbrian marked this pull request as ready for review August 24, 2022 10:00
@faustbrian
Copy link
Contributor Author

Just ran this against my app and its test suite and all good.

@faustbrian faustbrian requested review from holgerd77 and jochem-brouwer and removed request for holgerd77 August 24, 2022 10:27
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 too, won't give an approval yet.

/**
* Flushes all checkpoints, restoring the initial checkpoint state.
*/
flushCheckpoints() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@faustbrian
Copy link
Contributor Author

Will resolve conflicts when other PRs are sorted.

@faustbrian faustbrian changed the base branch from master to big-messy-bunch August 24, 2022 11:36
@faustbrian faustbrian merged commit e66c428 into big-messy-bunch Aug 24, 2022
@faustbrian faustbrian deleted the db-protected branch August 24, 2022 11:40
@holgerd77
Copy link
Member

@faustbrian ah, can you rather just cherry-pick the essential commits? These are only 7 or so, and then you could just solve some eventual conflicts one-by-one? With this muddling all together I am somewhat afraid that this will really head to some "big messy bunch". 😬

@holgerd77
Copy link
Member

(so on top of a fresh master?)

@holgerd77
Copy link
Member

Update: ah ok, maybe this works as well, we'll see. 😬

@faustbrian
Copy link
Contributor Author

Squashed everything into a new branch. Faster than cherry picking.

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.

2 participants