-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: eth_getProof
response
#12370
fix: eth_getProof
response
#12370
Conversation
@@ -7,7 +7,7 @@ use reth_trie_common::{AccountProof, StorageProof}; | |||
/// Creates a new rpc storage proof from a primitive storage proof type. | |||
pub fn from_primitive_storage_proof(proof: StorageProof) -> EIP1186StorageProof { | |||
EIP1186StorageProof { | |||
key: JsonStorageKey::Hash(proof.key), | |||
key: JsonStorageKey::Number(proof.key.into()), |
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.
hmm, I think this is more nuanced than that, because the reason why this even is an enum is so we can mirror the input:
so i guess we'd need to take this into account here:
.storage_proofs | ||
.into_iter() | ||
.zip(slots) | ||
.map(|(proof, slot)| from_primitive_storage_proof(proof, slot)) |
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.
@rkrasiuk is it OK to assume same order of input slots and outputted proofs here?
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.
it is the case rn, but i'd rather avoid such assumption since it's not documented anywhere, not covered in tests and there is no such expectation atm to proof generation
keys should always be serialized as uints