Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
ERC-7786 interfaces and receiver #5255
Changes from 59 commits
b2eedbe
efd2f30
bc42b25
07f4b44
40ba631
07ec518
95fb0db
f263819
f51fbe6
52a301b
027859e
a91a999
86abf5a
6dca3cb
a7a6e9e
ec9a659
568dc7b
0292c31
aea4a14
cf78a9f
26cec97
3a7f904
4d18729
c7a7c94
d6319e8
b3bf461
2ab63b7
231b93b
24f1490
43f0dc1
7b7c1fd
2abfa49
f433e6d
27c7c0d
75e1e4c
4f48757
1ec1e3f
53d72d7
c5790f8
f6007b0
b17f548
d679082
117c10f
54e1732
88ee5a4
811d7c3
c54c431
37f2363
1acd441
c5d6081
ec54368
d290223
6c1c61a
04f54c9
75cc5da
1dfff54
06f5493
1b45dbd
b62f8b5
5acc9ee
0c67669
23ce2c3
5fe2fb4
5ad9ad4
8f5c897
f2370bf
a158138
5ba182b
1aea26e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
shouldn't this check that
gatewayMessageKey
andgateway
are zero https://github.com/ethereum/ERCs/pull/673/files#diff-07848a22c16b11582927b37d76b2c54a0617b1444b30d11bca783ac98eaed01fR150? or is that checked by the gateway?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.
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.
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.
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.
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.
receiveMessage
is payable so it can receive value, but the value received is not passed tovalidateReceivedMessage
, how can the passive gateway verify themsg.value
received?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.
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.
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.
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)
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.
@frangio
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.
If we want to make payable compatible with passive mode we would have to add a
value
parameter tovalidateReceivedMessage
.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.
After discussion it feels like the thing is:
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 messageThere 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.
I would do
require(msg.value == 0)
. In terms of the spec I think this check should be recommended but not mandatory.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.
Added a check
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.
I think this should be
indexed
. I know it's not in the spec but we should change that.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.
I think we discussed it.
The MessageSent one is indexed (because we may want to activelly filter a particular MessageSent once we know the id. But for the
MessageCreated
part, we don't have a way to predict the value (at least not in general) so creating a is not that meaningfull IMO (hence why its not indexed)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.
My reasoning was that if you see a
MessageSent
event you may want to retrieve the matchingMessageCreated
.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.
make sens
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.
Why should it not emit a
MessageSent
event if above it says that when returning zero the message must be sent.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.
That is part of the ERC specification.
MessageSent(0)
would be useless (that is implicit by 0 being in MessageCreated) but also confusing (id 0 is not unique)