-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: add logsBloom class #2684
Conversation
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work just some Qs around
|
||
import { keccak256 } from 'ethereum-cryptography/keccak.js'; | ||
|
||
const BYTE_SIZE = 256; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to put this constant into a shared common place. I forgot what it is but there's a file we store constants, right?
if (!bitvector) { | ||
this.bitvector = new Uint8Array(BYTE_SIZE); | ||
} else { | ||
if (bitvector.length !== BYTE_SIZE) throw new Error('bitvectors must be 2048 bits long'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have a refference link states why logsBloom must be 2048 bits (256 bytes).
const mask = 2047; // binary 11111111111 | ||
|
||
for (let i = 0; i < 3; i++) { | ||
const first2bytes = new DataView(e.buffer).getUint16(i * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to explain along the steps through code comments
e = keccak256(e); | ||
const mask = 2047; // binary 11111111111 | ||
|
||
for (let i = 0; i < 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to explain along the steps through code comments
const mask = 2047; // binary 11111111111 | ||
let match = true; | ||
|
||
for (let i = 0; i < 3 && match; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to explain along the steps through code comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work unblocking this.
I think @quiet-node had some good requests in here.
This is one of those classes that is likely rarely going to be updated but his deep crypto logic that is good to make sure if well commented and referenced so that the 1 time we do have to update it we understand what's going on
Superseded by #2785 |
Description:
This PR adds logsBloom object, which can take care of all bloom calculations, needed when there is missing logsBloom. Example are synthetic transactions, where we don't have logsBloom from the mirror node response. This PR only adds the class for this, calculations will be done in another to keep them tight and focused.
Related issue(s):
Fixes #2683
Notes for reviewer:
Checklist