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

mpt: adjust verifyMPTProof naming and restructure index file #3730

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

gabrocheleau
Copy link
Contributor

This addresses the comment here: #3719 (comment)

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.37%. Comparing base (4470cc3) to head (8eb3bdd).
Report is 90 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain 83.49% <ø> (?)
client ?
common 89.85% <ø> (?)
tx ?
wallet ?

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

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@scorbajio scorbajio merged commit 29799e4 into master Oct 8, 2024
41 checks passed
@scorbajio scorbajio deleted the mpt/minor-fixes branch October 8, 2024 17:18
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.

Great, thanks for tackling so quickly! 🤩

Two somewhat additional things, can you maybe do in a follow-up PR if you agree with the updates?

}
}
}
export * from './proof.js'
Copy link
Member

Choose a reason for hiding this comment

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

This should re-export the range proof functionality as well.

proof,
opts?.useKeyHashingFunction ?? keccak256,
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, just seeing that this is included here! 🤯 Then on-top request, can we please:

  1. Move this fully over to range.ts and have one documented method here, likely kill off verifyRangeProof
  2. And then re-export verifyMerkleRangeProof from range.ts from within index.ts

@@ -240,7 +240,7 @@ export async function verifyMerkleStateProof(
const storageProof = stProof.proof.map((value: PrefixedHexString) => hexToBytes(value))
const storageValue = setLengthLeft(hexToBytes(stProof.value), 32)
const storageKey = hexToBytes(stProof.key)
const proofValue = await verifyMPTProof(storageKey, storageProof, {
const proofValue = await verifyMerkleProof(storageKey, storageProof, {
useKeyHashing: true,
})
const reportedValue = setLengthLeft(
Copy link
Member

Choose a reason for hiding this comment

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

Path related comment (so not for this file): can we align the proof directories from mpt and statemanager, and so (my suggestion) call this one here also:

  • statemanager/src/proofs -> statemanager/src/proof

(we have a lot of stuff in singular - e.g. also test (instead of tests), so this feels somewhat more as our "convention" here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the paths from mpt, it seems like we can actually do without src/proof and src/db.

  • Proof only exports two files
  • DB only exports one

So I moved them over to the src directory (renamed range.ts to rangeProof.ts). This flattens the repo structure and has the benefit of limiting the amount of nested index files.

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.

4 participants