-
Notifications
You must be signed in to change notification settings - Fork 2
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
Supra automation registry smart contract implementation within supra framework #119
base: feature/automation-networks
Are you sure you want to change the base?
Supra automation registry smart contract implementation within supra framework #119
Conversation
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,89 @@ | |||
/// Supra Automation Registry | |||
/// |
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.
public fun get_active_tasks() : vector<AutomationMetaData>
on_new_epoch()
should perform clean up and updation of state
Do we want to restrict only for entry function or should we allow scripts as well?
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
|
||
move_to(supra_framework, AutomationRegistry { | ||
current_index: 0, | ||
automation_gas_limit: DEFAULT_AUTOMATION_GAS_LIMIT, |
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 there should be separate type for config holding these values, and it should be read during initialization.
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.
Yes, it depends on how we are upgrading the automation_registry
with the SupraFramework.
If we are initializing through multisig, we will need to pass the parameter in that case. I will update accordingly once we finalize it.
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
// todo : pre-paid amount collect from the user | ||
// todo : duration/expiry in seconds | ||
// Expiry time does not go beyond upper cap duration set by admin/governance | ||
assert!(expiry_time < registry_data.duration_upper_limit, EEXPIRY_TIME_UPPER); |
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 check should be (expiry_time - now) < registry_data.duration_upper_limit
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 assume that the client will provide the expiration time in seconds, indicating how many days or months from now they need the execution to occur.
For example, if they want the execution to be valid for only one day, they would provide 86,400 seconds as an argument.
Please let me know if we should handle it in the manner you’ve outlined.
CC: Dr. @sjoshisupra
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.
Based on the last discussion I remember that we decided to have exact exipration-time in seconds.
Now
can be different in scope of different nodes, so global/absolute time is better than relative one. and this value also will be used filter out already expired automated-transactions in scope of SMR absolute time is preferable.
So I think even here we should take block-creation-time
or predicted_next_epoch_start_time
to check the actual duration, instead of current-time
// todo : gas_price_cap should not below chain minimum | ||
|
||
registry_data.current_index = registry_data.current_index + 1; | ||
|
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.
deposit_from_owner(owner)
- where automation fee calculation and withdrawal from owner account is done. should be done after all verification and before increasing index and registering the task.
26bc075
to
2392127
Compare
…xt_epoch` field, add more assersion in `register` function, also `get_active_task_ids` function will return only active task ids
2392127
to
d737095
Compare
…s_limit` and `update_duration_upper_limit`
…price_cap` should not be zero also collect static registry fee from user
…o include missing events
} | ||
|
||
/// Withdraw user's deposited fee from the resource account | ||
entry fun withdraw_fee(supra_framework: &signer, to: address, amount: u64) acquires AutomationRegistry { |
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.
should this function really be entry one? Isn't it utilized only in scope of this module in scope of remove_task
. If that's the case then here again registry_fee_address_signer_cap
can be passed as parameter to this function.
but here I have one question: @sjoshisupra how the accumulated coins will be later distributed.
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.
The withdraw_fee
function is invoked by the Supra Framework, enabling the framework admin to withdraw the automation task fees paid by users. However, this function is not utilized in the remove_task
process and directly access from outside of the SC
} | ||
|
||
/// Calculate and collect registry charge from user | ||
fun collect_from_owner(owner: &signer, _expiry_time: u64, _max_gas_amount: u64) { |
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.
This function will be called in scope of register
function where we already have registry
information, I think in order not to calculate registry fee address here again, it can be passed as parameter to this function.
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, we can do that
/// It's resource address which is use to deposit user automation fee | ||
registry_fee_address: address, | ||
/// Resource account signature capability | ||
registry_fee_address_signer_cap: SignerCapability, |
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.
we should have automation_unit_price
configuration property here which will define price per second or AutomationFeeParameters
struct were unit-price, threshold, etc. will be included.
/// Calculate and collect registry charge from user | ||
fun collect_from_owner(owner: &signer, _expiry_time: u64, _max_gas_amount: u64) { | ||
// todo : calculate and collect pre-paid amount from the user | ||
let static_amount = 100000000; // 1 Aptos |
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.
for the time being we can avoid applying dynamic charging, and we can collect flat rate based on the expiry-time and automation_unit_price
.
// Perform clean up and updation of state | ||
vector::for_each(ids, |id| { | ||
let task = enumerable_map::get_value_mut(&mut automation_registry.tasks, id); | ||
if (task.expiry_time < current_time) { |
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.
Here we should take into account also the tasks that are active during this new epoch but will be already expired for the next epoch. so we need to check (task.expire_time < current_time + epoch_timeframe)
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 assume that if a task does not have sufficient time duration to run within the epoch, and it expires in the middle of the epoch, such a task will not be considered for execution. Is that correct?
} | ||
|
||
/// Calculate and collect registry charge from user | ||
fun collect_from_owner(owner: &signer, _expiry_time: u64, _max_gas_amount: u64) { |
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.
can you please remind how the max_gas_amount will be utilized in automation fee calculation.
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.
Ohh, I see max_gas_amount
will be utilize for this purpose only
No description provided.