Change the API of execute_message
#4347
Conversation
MathieuDutSik
requested review from
afck,
bart-linera,
deuszx,
ma2bd and
ndr-ds
|
I'm not convinced this is a necessary change. Just like origin/sender etc are accessed via runtime similarly the origin chain id (and is_bouncing) can be there. In the API we have what's necessary and nothing more. Not every contract will use these new arguments and so they will have to ignore it. |
|
I like the argument |
Yes, for Now, regarding the
Therefore, my proposal is:
|
Definitely not. This is a waste of storage and network (because the information is already present) |
| /// The authenticated signer for this execution, if there is one. | ||
| fn authenticated_signer(&mut self) -> Result<Option<AccountOwner>, ExecutionError>; | ||
|
|
||
| /// If the current message (if there is one) was rejected by its destination and is now | ||
| /// bouncing back. | ||
| fn message_is_bouncing(&mut self) -> Result<Option<bool>, ExecutionError>; | ||
|
|
||
| /// The chain ID where the current message originated from, if there is one. | ||
| fn message_origin_chain_id(&mut self) -> Result<Option<ChainId>, ExecutionError>; | ||
|
|
||
| /// The optional authenticated caller application ID, if it was provided and if there is one | ||
| /// based on the execution context. | ||
| fn authenticated_caller_id(&mut self) -> Result<Option<ApplicationId>, ExecutionError>; |
There was a problem hiding this comment.
FYI these functions use to be in a Context structure and we moved them out because it felt cleaner (than passing around a context).
For this PR to go through, we need to agree that message-specific context values are an exception AND we don't plan to add more context values in the future for messages.
There was a problem hiding this comment.
Incidentally, we need to clarify in the doc what the origin of a bouncing message is.
There was a problem hiding this comment.
That seems indeed a big ask.
Because I can very well conceive that we pass more arguments to the function execute_message.
Potentially, all the entries of the MessageContext could be passed, and there is more than origin and is_bouncing. So, yes, we could add more context values.
Possible options:
- Have a
fn execute_message(context: Context, message: Vec<u8>)which leaves room for extending the values passed. - Just add the
is_bouncingentry.
| origin: ChainId, | ||
| message: Self::Message, | ||
| ) { | ||
| assert_eq!( |
There was a problem hiding this comment.
Note that this assert would be wrong on bouncing messages. So to be consistent, we might also add assert!(!is_bouncing, "This application doesn't use bouncing messages.")
There was a problem hiding this comment.
But, here is the problem: Shouldn't the assert statement be assert!(!is_bouncing, "Bouncing messages are not yet supported")?
Not processing bouncing messages is fine for Message::RemoveLiquidity and Message::RemoveAllAddedLiquidity since no operation of consequence is done before that needs to be reverted.
However, for Message::AddLiquidity and Message::Swap, there is a transfer operation before the message emission. So, what about that operation?
In all transfers, the transfers are done to the creator chain of the AMM. So, the creator chain of the AMM receives two kinds of messages:
- The ones of transfer
- The ones of the AMM.
I think the only reasonable design is that all messages are bounced or none of them (is that the case @afck?). Therefore, the transfers are reverted by the bouncing messages, and so it should be ok. And so the correct handling of bouncing messages for the AMM should be, I think
if is_bouncing {
return;
}Then the assert statement would work fine.
There was a problem hiding this comment.
An app cannot receive "bouncing messages" if it doesn't create "tracked messages".
If is_bouncing is confusing, then that's an argument in favor of hiding it until people need it.
There was a problem hiding this comment.
Thank you for correcting me.
I would argue that if it forces clarification on the is_bouncing then it is a desirable outcome.
But of course, we can abort the PR as well.
…-derive`. (#4419) ## Motivation It has recently been indicated in PR #4347 that the use `ChainId` in a message would be a waste of space. But there are other wastage of key space that can be reduced. Fixes #2391 ## Proposal We check for the number of entries in views obtained by `linera-views-derive`. If lower than 256, we convert to `u8`. It is likely that we need to remove as well the `linera_views::views::MIN_VIEW_TAG`. ## Test Plan CI. ## Release Plan It is definitely breaking the testnet. ## Links None
3 participants
Motivation
The function
execute_messagetakes a single argument. However, two additional arguments are implicitand can be accessed by the runtime:
Their return types
Option<bool>andOption<ChainId>reflect this. The solution is to have a singlefunction
execute_messagethat takes 3 arguments.Fixes #4271
Proposal
The solution was relatively straightforward to implement and yields a net simplification.
For the source of the message, several names are possible:
source_chain_id.message_origin_chain_id.chain_id.origin.I chose
originsince it is the terminology used inMessageContextand is the shortest one.The code of the smart contracts was not corrected. Ignored argument
is_bouncingare quite apparent.This should be corrected so that the smart contract handles the bouncing case correctly. But this is for further
work.
Test Plan
The CI.
Release Plan
Links
None.