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

chore: remove rlp encoding for Request #751

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented May 15, 2024

the EIP was a bit ambigious here, apparently we are not supposed to also RLP encode the result of the
EIP-7685 encoding. instead, to compute the request root, we should just stuff the EIP-7685 encoded request
into a list and RLP encode that list

this simply removes the unneeded RLP encoding. we could also have kept it around and simply do a self.encode_7685(buf), but I think that would break expectations around how RLP works (geth does this)

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

the EIP was a bit ambigious here, apparently we are not
supposed to also RLP encode the result of the
EIP-7685 encoding. instead, to compute the request root,
we should just stuff the EIP-7685 encoded request
into a list and RLP encode that list

this simply removes the unneeded RLP encoding
@onbjerg onbjerg added the debt Tech debt which needs to be addressed label May 15, 2024
@mattsse mattsse merged commit b408446 into main May 15, 2024
24 checks passed
@mattsse mattsse deleted the onbjerg/rm-rlp-request branch May 15, 2024 08:23
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
the EIP was a bit ambigious here, apparently we are not
supposed to also RLP encode the result of the
EIP-7685 encoding. instead, to compute the request root,
we should just stuff the EIP-7685 encoded request
into a list and RLP encode that list

this simply removes the unneeded RLP encoding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Tech debt which needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants