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

VM/SM/Blockchain/Trie/EVM: Copy() -> shallowCopy() function renaming #2826

Merged
merged 18 commits into from Jul 3, 2023

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented Jun 26, 2023

TODO:

@holgerd77
Copy link
Member

Quick note: after an exchange with @jochem-brouwer we realized that the EVM copy() method also has state and therefore should be renamed as well.

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #2826 (97c94cd) into master (49cf084) will decrease coverage by 2.75%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.16% <ø> (ø)
blockchain 92.56% <100.00%> (ø)
client ?
common 97.06% <100.00%> (ø)
devp2p 86.95% <ø> (ø)
ethash ∅ <ø> (∅)
evm 66.99% <100.00%> (ø)
rlp ∅ <ø> (∅)
statemanager 86.41% <100.00%> (ø)
trie 90.08% <100.00%> (+0.04%) ⬆️
tx ?
util 86.54% <ø> (ø)
vm 77.25% <100.00%> (ø)

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

@holgerd77
Copy link
Member

Did a branch update here via UI

@holgerd77
Copy link
Member

Another branch update via UI

@holgerd77 holgerd77 changed the title Copy function renaming Copy() -> shallowCopy() function renaming Jul 3, 2023
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 pretty good, thanks a lot! 🤩

One thing: it would be good if we give this a mention in the direct code docs above the - now - shallowCopy() methods - what the "shallow" part of the copy actually is, so that this gets a bit of a description, something like the following for EVM:

/**
* This method copies the EVM, current HF and EIP settings
* and returns a new EVM instance.
* 
* Note: this is only a shallow copy and both EVM instances
* will point to the same underlying state DB.
* 
* @returns EVMInterface
*/

Will merge though, this can be done in a small follow-up PR (also ok: alongside another PR).

@holgerd77 holgerd77 changed the title Copy() -> shallowCopy() function renaming VM/SM/Blockchain/Trie/EVM: Copy() -> shallowCopy() function renaming Jul 3, 2023
@holgerd77 holgerd77 merged commit 629c80f into master Jul 3, 2023
39 of 41 checks passed
@holgerd77 holgerd77 deleted the copy-function-renaming branch July 3, 2023 11:09
@scorbajio
Copy link
Contributor Author

Working on documentation update in #2855.

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.

Copy() -> shallowCopy() Copy Method Renaming for State Containing Libraries
3 participants