-
Notifications
You must be signed in to change notification settings - Fork 837
Update ERC-7786: Use interoperable binary addresses, improve attributes #1050
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
Conversation
|
✅ All reviewers have approved. |
|
Update ready but will be blocked by #1002. |
xinbenlv
left a comment
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.
Thanks for the improvements to ERC-7786! I have one specific question about event indexing.
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
|
@frangio , CAIP-350/ERC‑7930 is too immature for building other standards upon it. Currently it doesn't specify binary encoding for any namespace, even for EVM-compatible chains. So, to build other standards (such as ERC-7786) on top of CAIP-350/ERC‑7930 the following conditions should be met:
Otherwise there is no sense in migrating from CAIP-10 to less mature standard (CAIP-350/ERC‑7930) |
I disagree on this part. Parsing CAIP-10 ASCII strings on chain is so bad that it makes sense to migrate to a less mature standard. |
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
Co-authored-by: Hadrien Croubois <[email protected]>
33a8a05 to
b9497fe
Compare
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
|
Blocked on #1002. |
Head branch was pushed to by a user without write access
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
|
The commit cab9086 (as a parent of 87f4d8e) contains errors. |
| function receiveMessage( | ||
| bytes32 receiveId, // Unique identifier | ||
| bytes calldata sender, // Binary Interoperable Address | ||
| bytes calldata payload, |
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.
The comma here is redundant.
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
This PR includes the change to interoperable binary addresses from #1038, but preserves attributes with a small change, and does not add hooks.
To make clearer the fact that attributes are metadata strictly intended for the gateway, the attributes argument was removed from
receiveMessageand metadata will not be forwarded to the receiver.The rationale for preserving a structured form of metadata rather than opaque
extraDatais:extraDatawould rely entirely on off-chain consensus and coordination on encoding and semantics between the sender and the gateway. This prevents on-chain constructions that are able to detect errors and adapt to gateway functionality.bytes4selector that is prepended to the actual metadata values. The gateway can use this to safely detect that it supports the metadata that was passed in, which could otherwise successfully decode but be misinterpreted.supportsMetadata(bytes4). For an example, consider a sender contract that supports multiple ways of specifying gas payment for the call and can choose which one to use.bytes[]) as opposed to a single piece of metadata (bytes extraData) allows multiple extensions to be combined without disrupting the other metadata and requiring less coordination.The rationale for not adding hooks is:
getTransientMessageContext()getter in the gateway, as currently specified the gateway address has to be provided by the sender in the hook data and the hook must trust it.hook(address,bytes,uint256)triggers a hook call.