Skip to content

Bring 2537 back#3350

Merged
acolytec3 merged 17 commits into
masterfrom
bring-2537-back
Apr 10, 2024
Merged

Bring 2537 back#3350
acolytec3 merged 17 commits into
masterfrom
bring-2537-back

Conversation

@acolytec3

@acolytec3 acolytec3 commented Apr 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Brings back our original EIP implementation of 2537.

  • Reverts commit 31aa264
  • Updates mcl-wasm to latest versions
  • Removes browser exceptions
  • Adds tests using test vectors from EIP

@acolytec3 acolytec3 requested a review from jochem-brouwer April 8, 2024 17:33
jochem-brouwer
jochem-brouwer reviewed Apr 9, 2024
View reviewed changes

@jochem-brouwer jochem-brouwer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments. We should also add more tests! (Or import / test locally by using old ethereum/tests vectors). Now the precompiles are not tested 🤔

Comment thread packages/evm/src/evm.ts Outdated
Comment thread packages/evm/src/evm.ts Outdated
typeof window === 'undefined' ? process?.env?.DEBUG?.includes('ethjs') ?? false : false
}

protected async init(): Promise<void> {

@jochem-brouwer jochem-brouwer Apr 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we remove this method and put this into evm.create?

Comment thread packages/evm/src/precompiles/index.ts Outdated
precompile: precompile0a,
name: 'KZG (0x0a)',
},
// 0x00..0b: beacon block root, see PR 2810

@jochem-brouwer jochem-brouwer Apr 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where did the beacon block root precompile go to?

@acolytec3 acolytec3 Apr 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question 🤔 Will have to look

Comment thread packages/evm/src/precompiles/util/bls12_381.ts
jochem-brouwer
jochem-brouwer approved these changes Apr 10, 2024
View reviewed changes

@jochem-brouwer jochem-brouwer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@codecov

codecov Bot commented Apr 10, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (4be68d2) to head (0f25033).
Report is 2 commits behind head on master.

❗ Current head 0f25033 differs from pull request most recent head 4252d63. Consider uploading reports for the commit 4252d63 to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client ?
common 94.17% <100.00%> (+0.14%) ⬆️
devp2p ?
evm ?
genesis ?
statemanager ?
trie ?
tx ?
util ?
vm ?
wallet ?

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

@acolytec3 acolytec3 merged commit df50617 into master Apr 10, 2024
@holgerd77 holgerd77 deleted the bring-2537-back branch April 11, 2024 07:48
@holgerd77 holgerd77 mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@holgerd77 holgerd77 holgerd77 left review comments
@jochem-brouwer jochem-brouwer jochem-brouwer approved these changes

Assignees

No one assigned

Labels

package: common package: evm package: vm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@acolytec3 @holgerd77 @jochem-brouwer