[AN-Issue-1435] Enable epoch-interval and last-configuration-time properties in registry #158
Conversation
…tion registry module
…le module and then remove registry state module
nizam-supraoracles
changed the title
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/reconfiguration.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/reconfiguration.move
Outdated
Show resolved
Hide resolved
| tasks: EnumerableMap<u64, AutomationTaskMetaData>, | ||
| current_index: u64, | ||
| gas_committed_for_next_epoch: u64, | ||
| automation_gas_limit: u64, |
There was a problem hiding this comment.
If we want to keep AutomationRegistryState as separate struct then I would suggest to remove automation_gas_limit from it's context, otherwise we can flatten and have it embedded in AutomationRegistry.
| @@ -51,6 +82,33 @@ module supra_framework::automation_registry { | |||
| registry_fee_address: address, | |||
| /// Resource account signature capability | |||
| registry_fee_address_signer_cap: SignerCapability, | |||
| /// Time period between epochs. | |||
There was a problem hiding this comment.
If it is acceptable I would suggest to have a struct enclosing all the properties related to epochs to make the AutomationRegistry more compact
EpochState {
/// Epoch expected duration at the beginning of the new epoch, Based on this and actual
/// epoch-duration which will be (current-time - last_reconfiguration_time) automation tasks
/// refunds will be calculated.
/// it will be updated upon each new epoch start with epoch_interval value.
/// Although we should be careful with refunds if block production interval is quite high.
expected_epoch_duration: u64
/// Epoch interval that can be updated any moment of the time
epoch_interval: u64,
/// Current epoch start time which is the same as last_reconfiguration_time
start_time: u64,
}
There was a problem hiding this comment.
As we are going to introduce AutomationRegistryConfig as mentioned in the https://github.com/Entropy-Foundation/smr-moonshot/issues/1436 , I have plan to do this with the different PR, if it's okay.
| let state = borrow_global_mut<AutomationRegistryState>(@supra_framework); | ||
| let ids = enumerable_map::get_map_list(&state.tasks); | ||
|
|
||
| automation_registry.last_reconfiguration_time = last_reconfiguration_time; |
There was a problem hiding this comment.
last_reconfiguration_time should be updated at the very end of this function.
also we can avoid passing it as input parameter as it is going to be the same as timestamp::now_seconds()
| let automation_base_fee = task_duration * automation_unit_price; | ||
| // todo : dynamic price calculation is pending | ||
| supra_account::transfer(owner, registry_fee_address, automation_base_fee); | ||
| } | ||
|
|
||
| /// Get last epoch time in second | ||
| fun get_last_epoch_time_second(): u64 { | ||
| let last_epoch_time_ms = reconfiguration::last_reconfiguration_time() / MILLISECOND_CONVERSION_FACTOR; | ||
| fun get_last_epoch_time_second(last_reconfiguration_time: u64): u64 { |
There was a problem hiding this comment.
I think this function can be removed already.
| let registry_state_data = borrow_global_mut<AutomationRegistryState>(@supra_framework); | ||
| let registration_time = timestamp::now_seconds(); | ||
|
|
||
| assert!(expiry_time > registration_time, EINVALID_EXPIRY_TIME); |
There was a problem hiding this comment.
Let's have similar checks close to each other.
… field + remove `last_reconfiguration_time` from on_new_epoch + remove `get_last_epoch_time_second` unused function + update testcases
Resolve https://github.com/Entropy-Foundation/smr-moonshot/issues/1435