-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Automation: MoveLayer] Implement AutomationRegistryConfig #167
[Automation: MoveLayer] Implement AutomationRegistryConfig #167
Conversation
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 can see that new configuration parameters related to automation-fee are not included in this PR. If you are planning to add them as update to this PR that's fine. I have updated the issue description to contain all the list of the parameters that we have discussed so far.
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
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.
Looks good to me. Although I have couple of comments.
2_626_560, | ||
100_000_000, | ||
1000, | ||
500000000, // 5 supra |
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.
isn't 5 supra to much ?
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.
It's 500,000,000 quants, which is equivalent to 5 Supra. Were you referring to something else? I'm not sure if I misunderstood
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, I am asking is 5 supra is reasonable amount for flat registration fee. By bad , there was a typo in previous comment and I fixed it :(
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.
If the current flat automation registration fee is too high, we could consider setting it to the cost of 1 Supra. Alternatively, what fee structure do you and Dr. Joshi think is appropriate?
@@ -210,6 +250,20 @@ module supra_framework::automation_registry { | |||
} | |||
}); | |||
|
|||
// Apply the latest configuration if any parameter has been updated. | |||
if (config_buffer::does_exist<AutomationRegistryConfig>()) { |
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.
let's have a separate function for config update.
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.
created new internal function update_config_from_buffer
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.
Is it expected to be part of this PR ?
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, since https://github.com/Entropy-Foundation/smr-moonshot/issues/1436 this issue includes the implementation of config-buffer 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/cached-packages/src/aptos_framework_sdk_builder.rs
Outdated
Show resolved
Hide resolved
aptos-move/framework/supra-framework/sources/automation_registry.move
Outdated
Show resolved
Hide resolved
@@ -210,6 +250,20 @@ module supra_framework::automation_registry { | |||
} | |||
}); | |||
|
|||
// Apply the latest configuration if any parameter has been updated. | |||
if (config_buffer::does_exist<AutomationRegistryConfig>()) { |
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.
Is it expected to be part of this PR ?
… to handle configrable data
…yConfig` and replace the data after on_new_epoch
… default value, make relavent test cases update
…te `check_registration_task_duration` to check necessory time related operation
… we have initialization flow properly implemented
…onRefund` and add task_index + create common `config_update_from_buffer` internal function + add expirty_time>current_time condition in to `refund_automation_task_fee` fun
…n_task_metadata.expiry_time > current_time`, otherwise do not fail the tx
a453a57
to
31b1314
Compare
37417cf
into
feature/automation-networks
Implementation of https://github.com/Entropy-Foundation/smr-moonshot/issues/1436