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

ERC-7786 interfaces and receiver #5255

Closed
wants to merge 69 commits into from

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Oct 14, 2024

Depends on #5252

ERC PR

NOTE

Bytes.slice uses mcopy that was introduced in 0.8.25. This requirement extends to the CAIP2 and CAIP10 library

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@Amxx Amxx requested a review from cairoeth October 16, 2024 12:52
@Amxx Amxx added this to the 5.2 milestone Oct 16, 2024
contracts/crosschain/draft-ERC7786Receiver.sol Outdated Show resolved Hide resolved
contracts/crosschain/draft-ERC7786Receiver.sol Outdated Show resolved Hide resolved
contracts/crosschain/draft-ERC7786Receiver.sol Outdated Show resolved Hide resolved
contracts/crosschain/draft-ERC7786Receiver.sol Outdated Show resolved Hide resolved
contracts/crosschain/draft-ERC7786Receiver.sol Outdated Show resolved Hide resolved
@cairoeth
Copy link
Contributor

Bytes.slice is using mcopy in assembly so it works on 0.8.24, so want to confirm that we are upgrading to 0.8.25 for mcopy in code generation

) public payable virtual {
if (_isKnownGateway(msg.sender)) {
// Active mode
// no extra check
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this check that gatewayMessageKey and gateway are zero https://github.com/ethereum/ERCs/pull/673/files#diff-07848a22c16b11582927b37d76b2c54a0617b1444b30d11bca783ac98eaed01fR150? or is that checked by the gateway?

Copy link
Collaborator Author

@Amxx Amxx Oct 16, 2024

Choose a reason for hiding this comment

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

As far as I remember the ERC says that in active mode the fields should be empty, not that must be empty. The idea is that we should not expect them to be populated (in fact we don't use these values in active mode)... but we should also not fail if they contain something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I specified it as "SHOULD" so this implementation is compliant. But it's worth considering if it should be "MUST". I don't see any concrete issue with allowing any values and ignoring them.

// no extra check
} else if (_isKnownGateway(gateway)) {
// Passive mode
IERC7786GatewayDestinationPassive(gateway).validateReceivedMessage(
Copy link
Contributor

Choose a reason for hiding this comment

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

receiveMessage is payable so it can receive value, but the value received is not passed to validateReceivedMessage, how can the passive gateway verify the msg.value received?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In passive mode, the value is not sent by the relayer. Value (if any) must be sent by the gateway.

So if you expect value with a message, and if you operate in passive mode, you should have a receive function. When you do the validate, the gateway should send you the value using that receive function.

How the receiver detect that value was received is a great question. I think it's more a question for the ERC than for the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One answer is that any value should come with a corresponding attribute, and that is how it would be visible from the processMessage (instead of using msg.value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make payable compatible with passive mode we would have to add a value parameter to validateReceivedMessage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussion it feels like the thing is:

  • Value should only be part of active mode calls.
  • Passive mode should not support value, because value cannot be easily moved in a passive way
    • In that case we have a choice of doing require(msg.value == 0); in the passive mode, or to allow value to be passed by the relayer, without it being related to the semantics of the message

Copy link
Contributor

Choose a reason for hiding this comment

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

I would do require(msg.value == 0). In terms of the spec I think this check should be recommended but not mandatory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a check

.changeset/khaki-keys-burn.md Outdated Show resolved Hide resolved
@Amxx
Copy link
Collaborator Author

Amxx commented Oct 16, 2024

Bytes.slice is using mcopy in assembly so it works on 0.8.24, so want to confirm that we are upgrading to 0.8.25 for mcopy in code generation

Right, I misread the changelog, will go back to 0.8.24 for Bytes and all the code that depends on it directly or indirectly.

Co-authored-by: cairo <[email protected]>
Co-authored-by: Arr00 <[email protected]>
.changeset/khaki-keys-burn.md Outdated Show resolved Hide resolved
@Amxx
Copy link
Collaborator Author

Amxx commented Oct 22, 2024

Because the discussion on the PR is still quite active, we (@frangio and I) decided to remove that from the main repo in 5.2. This will go in the community repo, where it will be more "fluid".

Basically, we don't want to publish helpers that we know will be outdated in a few weeks.

@Amxx Amxx closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants