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

develop: ci fix, some last backwards compatibility removals #1834

Merged
merged 13 commits into from
Apr 5, 2022

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Apr 5, 2022

This PR fixes an issue in develop where a karma workaround was erroring in the hardhat e2e installation for our packages. I moved the fix to the blockchain karma config.

I also executed some final breaking change items in VM and Trie.

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #1834 (5e9da55) into develop (d2c6702) will increase coverage by 0.05%.
The diff coverage is 95.74%.

Impacted file tree graph

Flag Coverage Δ
block 85.17% <ø> (ø)
blockchain 83.13% <ø> (ø)
client 77.29% <100.00%> (-0.02%) ⬇️
common 94.94% <ø> (ø)
devp2p 82.96% <ø> (+0.33%) ⬆️
ethash 90.71% <ø> (ø)
trie 80.35% <75.00%> (+0.07%) ⬆️
tx 92.48% <ø> (ø)
util 89.43% <ø> (ø)
vm 82.83% <97.29%> (+0.05%) ⬆️

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

@ryanio
Copy link
Contributor Author

ryanio commented Apr 5, 2022

I noticed a series of my commits missing from my prior PR #1815:

Screen Shot 2022-04-04 at 9 25 38 PM

Highlighted here:

Screen Shot 2022-04-04 at 9 26 15 PM

So I have cherry-picked them and added them back to this PR.

@holgerd77
Copy link
Member

Rebased this via UI with a local test rebase before.

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.

Nice PR 🙂, looks good! 👍

@@ -284,9 +283,7 @@ export default class VM extends AsyncEventEmitter {
if (opts.stateManager) {
this.stateManager = opts.stateManager
} else {
const trie = new Trie()
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -104,7 +104,7 @@ export class Trie {
*/
async checkRoot(root: Buffer): Promise<boolean> {
try {
const value = await this._lookupNode(root)
const value = await this.lookupNode(root)
Copy link
Member

Choose a reason for hiding this comment

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

We didn't plan to do a breaking Trie release initially, but my previously somewhat stronger opinion on this is wayning, so we might just do along (and then also finally do the renaming).

Just wanted to mention here. We then eventually want to have one additional thought round or broader look on other breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

So, some additional context: currently Trie is still listed in the "Future Release Planning" issue: #1718. Should I move over? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry didn't notice that.

yeah sure let's include these refactorings, i think now is the time to do it while we are preparing develop. not sure if it makes sense to wait any longer. we can assign some team members to the tasks and i can also dedicate some time this month to tackle them.

])
)
)
receiptsSize += Buffer.byteLength(receiptsBuffer)
Copy link
Member

Choose a reason for hiding this comment

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

Could you drop a short note what's happening here?

Does this fix some concrete issues? Can't oversee this on a first look.

Copy link
Contributor Author

@ryanio ryanio Apr 5, 2022

Choose a reason for hiding this comment

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

ah yeah, actually you commented on an old version of this code, if you look at the final state in this PR i made it much more simple by using our existing encodeReceipt function. this is simply keeping track of the size of the GetReceipts devp2p response, so we can respect capping at 10mib as the spec suggests.

@holgerd77 holgerd77 merged commit 3fc16da into develop Apr 5, 2022
@holgerd77 holgerd77 deleted the develop-ci-fix branch April 5, 2022 08:57
holgerd77 pushed a commit that referenced this pull request Apr 5, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
holgerd77 pushed a commit that referenced this pull request Apr 7, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
g11tech pushed a commit that referenced this pull request Apr 29, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
holgerd77 pushed a commit that referenced this pull request May 4, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
g11tech pushed a commit that referenced this pull request Jun 3, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
holgerd77 pushed a commit that referenced this pull request Jun 8, 2022
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
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