-
Notifications
You must be signed in to change notification settings - Fork 1
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: use send_gateway_tx
on payload upload
#13
Conversation
This way we can ignore the error in case the account has already been initialized. Signed-off-by: Guilherme Felipe da Silva <[email protected]>
Signed-off-by: Guilherme Felipe da Silva <[email protected]>
Signed-off-by: Guilherme Felipe da Silva <[email protected]>
495549c
to
dfbde96
Compare
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.
LGTM! - but wait for others who have more relayer context ;)
let incoming_message = IncomingMessage::read(&raw_incoming_message) | ||
.ok_or_eyre("failed to read incoming message")?; | ||
|
||
Ok(incoming_message.status == MessageStatus::Executed) |
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 just wonder if this would be a relatively expensive operation (One io RPC call) and how far we would be from implementing an LRU cache. I guess this need all depends how many "hits" we are expecting here. If we are hitting this check all the time, i would maybe go for it (on a separate issue). Wdyt?
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 the standard flow we'd get 1 hit per 1 message upon relayer startup, no extra calls. I don't think LRU cache would help us here.
This is because the relayer gets "tasks" from the Amplifier API that are always sequential and always unique. This means that there's only ever gonna be a single "execute GMP message" task for a single message incoming message (unless the relayer gets restarted and we query the same tasks from the Amplifier API -- but even then LRU cache would not help us 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.
Yeah, I'm not sure how caching would help us here. I mean, it would be nice to be able to avoid this call, but we would likely have a cache miss most of the time and thus the cost of checking the cache would not be worth it.
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.
Okay i see. So the amplifier API only works with "exactly once" and not with "at least once" semantics. Thanks !
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.
lgtm
let incoming_message = IncomingMessage::read(&raw_incoming_message) | ||
.ok_or_eyre("failed to read incoming message")?; | ||
|
||
Ok(incoming_message.status == MessageStatus::Executed) |
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 the standard flow we'd get 1 hit per 1 message upon relayer startup, no extra calls. I don't think LRU cache would help us here.
This is because the relayer gets "tasks" from the Amplifier API that are always sequential and always unique. This means that there's only ever gonna be a single "execute GMP message" task for a single message incoming message (unless the relayer gets restarted and we query the same tasks from the Amplifier API -- but even then LRU cache would not help us here)
Describe the changes
This way we can ignore the error in case the account has already been initialized and/or in case the message has already been committed.
Also, added check to avoid uploading the message and calling the destination program in case the
IncomingMessage
has already been executed.Related Issue(s)
Closes #12