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

[AN-Issue-1347] Implemented Automated transaction execution flow #136

Draft
wants to merge 1 commit into
base: task/issue-1346
Choose a base branch
from

Conversation

aregng
Copy link

@aregng aregng commented Dec 11, 2024

No description provided.

txn_gas_price: u64,
txn_max_gas_units: u64,
txn_expiration_time: u64,
_script_hash: vector<u8>,
Copy link

@nizam-supraoracles nizam-supraoracles Dec 12, 2024

Choose a reason for hiding this comment

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

In the old prologue functions, I noticed that we pass chain_id and validate it using an assertion, such as assert!(chain_id::get() == chain_id, error::invalid_argument(PROLOGUE_EBAD_CHAIN_ID)).
Do we need this validation in the automated_transaction_prologue?

BTW, there is a typo in automated_transaction_prologuetranaction.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the hint. also _script_hash should be removed from here.
as for chain-id, it can be there or not.
AutomatedTransaction will be constructed internally by smr-moonshot-executor component, and the chain ID will be set there. so theoretically it should be always valid.
But as long as smr-moonshot-executor and MoveVM are completly isolated components, and one provides input to the other, in my understanding, each component should validate its input and not rely on the fact that the other one will not produce any buggy information. This way the if there is a bug in one layer which is not tested, then the other layer can at least point it out.
But we need to agree whether we want to go with this approach or not.

Copy link

@sjoshisupra sjoshisupra left a comment

Choose a reason for hiding this comment

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

@aregng , it seems that from the issue 1347 I did not see

  • Chain ID check being done (if it is not done for other tx, we may not do it for automation as well)
  • Check that there is a corresponding active task for this tx (each automation task would have a unique id provided by registry) and sender seems missing. If we assume however that nodes are honest I wonder if this check is required though.

Comment on lines +178 to +181
assert!(
timestamp::now_seconds() < txn_expiration_time,
error::invalid_argument(PROLOGUE_ETRANSACTION_EXPIRED),
);

Choose a reason for hiding this comment

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

I was under the impression that if an automation task is beyond it's expiry, the SMR layer would never include it in the set of tx to be executed. Is that still correct? Because otherwise it would result in continuous failed assert here in the prologue.

Copy link
Author

Choose a reason for hiding this comment

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

That is true, but as mentioned in my previous comment, I think each layer should validate its input and not rely on the other component correctness. If we do not agree with this approach and here we will always assume that input will not be expired, then this check can be removed.

@aregng
Copy link
Author

aregng commented Dec 12, 2024

@aregng , it seems that from the issue 1347 I did not see

  • Chain ID check being done (if it is not done for other tx, we may not do it for automation as well)
  • Check that there is a corresponding active task for this tx (each automation task would have a unique id provided by registry) and sender seems missing. If we assume however that nodes are honest I wonder if this check is required though.
  • ChainID, During pervious discussion it was marked as optional, that's why I have not add it and wanted to clarify one more time. The check is done for user-transaction. So I will add it here as well.
  • Regarding check against registry: As mentioned in the above comment, either we always trust that the input provided by other component can not be incorrect, or each component does some checks on its own as well.
    Can you point where sender is missing ?

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.

3 participants