[AN-Issue-1409] Handled automation-registry state update on new epoch #144

aregng · 2024-12-18T09:12:13Z

split automation registry external API and internal state to avoid circular dependencies
enabled automation-registry-state update on new epoch from block module.

aptos-move/framework/supra-framework/sources/automation_registry_state.move

nizam-supraoracles

I have just one question regarding the removal of txn_app_hash; everything else looks good to me.

aptos-move/framework/supra-framework/sources/transaction_context.move

aptos-move/framework/supra-framework/sources/automation_registry.move

sjoshisupra · 2024-12-18T09:19:14Z

aptos-move/framework/supra-framework/sources/automation_registry.move

-    /// Remove automation task registry event
-    struct RemoveAutomationTask has drop, store {
-        id: u64
-    }

    // todo : this function should call during initialzation, but since we already done genesis in that case who can access the function


This can still be called by governance, after framework upgrade by governance.

sjoshisupra · 2024-12-20T10:03:42Z

aptos-move/framework/supra-framework/sources/automation_registry_state.move

+        /// A collection of automation task entries that are active state.
+        tasks: EnumerableMap<u64, AutomationTaskMetaData>,
+        current_index: u64,
+        gas_committed_for_next_epoch: u64,
+        automation_gas_limit: u64,
+    }


Will all governance configurable parameter also go here? Or are we going to have a separate config/automation_config.move similar to staking_config? For example, upper limit on duration, base fee, congestion_threshold, congestion_multiplier all should be governance configurable parameters.

at this moment parameters has been split into 2 contexts automation_registry and automation_registry_state based on their place of utilization. but I think as long as we are going to have more and more configurable parameters it would be better to have dedicated structure for it. But I would suggest to do it in a separate PR e.g in scope of the PR wehn automation-fee related configurations will be introduced.

aptos-move/framework/supra-framework/sources/automation_registry_state.move

aptos-move/framework/supra-framework/sources/automation_registry.move

sjoshisupra · 2024-12-20T10:27:13Z

aptos-move/framework/supra-framework/sources/block.move

@@ -235,6 +236,7 @@ module supra_framework::block {
        randomness::on_new_block(&vm, epoch, round, option::none());
        if (timestamp - reconfiguration::last_reconfiguration_time() >= epoch_interval) {
            reconfiguration::reconfigure();
+            automation_registry_state::on_new_epoch(epoch_interval);


This should be called from reconfiguration

reconfiguration does not have access to epoch-interval, either reconfiguration::reconfigure() should be updated to accept epoch_interval as parameter or we should leave it as it is.

sjoshisupra · 2024-12-20T10:27:39Z

aptos-move/framework/supra-framework/sources/block.move

@@ -264,6 +266,7 @@ module supra_framework::block {

        if (timestamp - reconfiguration::last_reconfiguration_time() >= epoch_interval) {
            reconfiguration_with_dkg::try_start();
+            automation_registry_state::on_new_epoch(epoch_interval);


should be called from reconfiguration

- split automation registry external API and internal state to avoid circular dependencies - enabled automation-registry-state update on new epoch from block module.

…s argument - Moved basic parameters checks of registration to automation_registry_state - Added and updated unit tests of autmation_registry_state

sjoshisupra · 2024-12-23T12:20:01Z

aptos-move/framework/supra-framework/sources/automation_registry.move

-
-        // Adjust the gas committed for the next epoch by subtracting the gas amount of the expired task
-        automation_registry.gas_committed_for_next_epoch = automation_registry.gas_committed_for_next_epoch - expired_task_gas;
-    }

    /// Withdraw accumulated automation task fees from the resource account - access by admin
    entry fun withdraw_automation_task_fees(


Yes, any governance action can only be taken through a script, so the interface would be public functions that checks that signer is indeed supra_framework

aptos-move/framework/supra-framework/sources/automation_registry.move

sjoshisupra · 2024-12-23T12:29:37Z

aptos-move/framework/supra-framework/sources/automation_registry_state.move

+        id: u64
+    }
+
+    public fun task_expiry_time(task: &AutomationTaskMetaData): u64 {


Suggested change

public fun task_expiry_time(task: &AutomationTaskMetaData): u64 {

#[view]

public fun task_expiry_time(task: &AutomationTaskMetaData): u64 {

this is internal function and not a view function exposed to user.

I believe that a [view] function can only call other [view] functions. Therefore, even if we want to call this from other module, this should be declared as [view] function. If we do not want to expose this to public, perhaps this can be public(friend)

aptos-move/framework/supra-framework/sources/automation_registry_state.move

sjoshisupra · 2024-12-23T12:41:48Z

aptos-move/framework/supra-framework/sources/automation_registry_state.move

+        // Adjust the gas committed for the next epoch by subtracting the gas amount of the cancelled task
+        state.gas_committed_for_next_epoch = state.gas_committed_for_next_epoch - automation_task_metadata.max_gas_amount;
+
+        event::emit(CanclledAutomationTask { id: automation_task_metadata.id });


Suggested change

event::emit(CanclledAutomationTask { id: automation_task_metadata.id });

event::emit(CancelledAutomationTask { id: automation_task_metadata.id });

Upon cancellation some event should also emit information about amount refunded

I assume when coin transfer happens, there will be deposit event, wouldn;t that be enough or we want to have it part of cancellation event as well along with id?

aptos-move/framework/supra-framework/sources/automation_registry_state.move

sjoshisupra · 2024-12-23T12:46:52Z

aptos-move/framework/supra-framework/sources/automation_registry_state.move

+        let active_task_ids = vector[];
+        let ids = enumerable_map::get_map_list(&state.tasks);
+
+        vector::for_each(ids, |id| {


Since we are only reading the state, perhaps vector::for_each_ref can be used to avoid creation/copy of elements.

@nizam-supraoracles

aptos-move/framework/supra-framework/sources/automation_registry_state.move

sjoshisupra · 2024-12-24T07:13:55Z

aptos-move/framework/supra-framework/sources/automation_registry_state.move

@@ -89,7 +92,7 @@ module supra_framework::automation_registry_state {

    #[event]
    /// Remove automation task registry event
-    struct RemoveAutomationTask has drop, store {
+    struct CanclledAutomationTask has drop, store {


CancelledAutomationTask (spelling)

aregng requested review from nizam-supraoracles and sjoshisupra December 18, 2024 09:12

nizam-supraoracles approved these changes Dec 18, 2024

View reviewed changes

nizam-supraoracles reviewed Dec 19, 2024

View reviewed changes

aptos-move/framework/supra-framework/sources/automation_registry_state.move Outdated Show resolved Hide resolved

aptos-move/framework/supra-framework/sources/automation_registry_state.move Show resolved Hide resolved

nizam-supraoracles approved these changes Dec 20, 2024

View reviewed changes

aptos-move/framework/supra-framework/sources/transaction_context.move Show resolved Hide resolved

aptos-move/framework/supra-framework/sources/automation_registry.move Outdated Show resolved Hide resolved

aregng force-pushed the feature/automation-networks branch from 6e99cf6 to bb6fb1b Compare December 20, 2024 10:27

sjoshisupra requested changes Dec 20, 2024

View reviewed changes

Aregnaz Harutyunyan and others added 5 commits December 20, 2024 16:32

Handled automation-registry state update on new epoch

0be027f

- split automation registry external API and internal state to avoid circular dependencies - enabled automation-registry-state update on new epoch from block module.

Addressed review comments

104ab5b

Added couple of unit-tests for automation-registry-state

335a455

Updated automation-registry to accept registration transaction hash a…

6853963

…s argument - Moved basic parameters checks of registration to automation_registry_state - Added and updated unit tests of autmation_registry_state

Removed redundant imports.

bc106d9

aregng force-pushed the task/registry-update-on-new-epoch branch from ab774df to bc106d9 Compare December 20, 2024 12:32

Addressed review comments

254a154

nizam-supraoracles approved these changes Dec 23, 2024

View reviewed changes

sjoshisupra requested changes Dec 23, 2024

View reviewed changes

Addressed 2nd round of review comments

7b949fb

sjoshisupra approved these changes Dec 24, 2024

View reviewed changes

Aregnaz Harutyunyan added 2 commits December 24, 2024 11:36

Fixed typo

dfe4aa8

Addressed review comments

3c110db

aregng merged commit a978ff6 into feature/automation-networks Dec 24, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AN-Issue-1409] Handled automation-registry state update on new epoch #144

[AN-Issue-1409] Handled automation-registry state update on new epoch #144

aregng commented Dec 18, 2024

nizam-supraoracles left a comment

sjoshisupra Dec 18, 2024

sjoshisupra Dec 20, 2024

aregng Dec 20, 2024

sjoshisupra Dec 20, 2024

aregng Dec 20, 2024

sjoshisupra Dec 20, 2024

sjoshisupra Dec 23, 2024

sjoshisupra Dec 23, 2024

aregng Dec 23, 2024

sjoshisupra Dec 24, 2024

sjoshisupra Dec 23, 2024

aregng Dec 23, 2024

sjoshisupra Dec 23, 2024

sjoshisupra Dec 24, 2024

sjoshisupra Dec 24, 2024

	public fun task_expiry_time(task: &AutomationTaskMetaData): u64 {
	#[view]
	public fun task_expiry_time(task: &AutomationTaskMetaData): u64 {

	event::emit(CanclledAutomationTask { id: automation_task_metadata.id });
	event::emit(CancelledAutomationTask { id: automation_task_metadata.id });

[AN-Issue-1409] Handled automation-registry state update on new epoch #144

[AN-Issue-1409] Handled automation-registry state update on new epoch #144

Conversation

aregng commented Dec 18, 2024

nizam-supraoracles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment