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

feat(sequencer): implement oracle module actions #1878

Open
wants to merge 12 commits into
base: feat/oracle
Choose a base branch
from

Conversation

noot
Copy link
Collaborator

@noot noot commented Dec 13, 2024

Summary

implement oracle module actions AddCurrencyPair and RemoveCurrencyPair

see https://github.com/skip-mev/connect/blob/main/proto/connect/oracle/v2/tx.proto and https://github.com/skip-mev/connect/blob/9ea31680774e2f71e683c0a4989df5bf2d5f2302/x/oracle/keeper/msg_server.go#L28 for relevant connect code

Background

these are required for the oracle authorities to be able to add/remove currency pairs from state; these are the pairs whose prices will be included in VEs.

Changes

  • implement oracle module actions AddCurrencyPair and RemoveCurrencyPair
  • make CurrencyPairState.QuotePrice optional, to reflect that this may not be set upon currency pair initialization (as no oracle price has been received for it)
  • remove get/put_num_removed_currency_pairs from the oracle state_ext as it's not needed for our logic due to the fact that we execute txs in the proposal phase, so by the time vote extensions are issued for a block, the state has already been updated and currency pairs that are removed in that block have already been removed.

Testing

unit tests

Breaking Changelist

  • adds new actions to the sequencer
  • sequencer genesis is also updated (new fees)

Related Issues

part of # #1446

@github-actions github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Dec 13, 2024
@noot noot marked this pull request as ready for review December 13, 2024 17:34
@noot noot requested review from a team and joroshiba as code owners December 13, 2024 17:34
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

There are a couple of blocking issues in the action handlers, but mostly LGTM.

@@ -55,6 +57,9 @@ impl_belong_to_group!(
(FeeAssetChange, Group::BundleableSudo),
(IbcRelay, Group::BundleableGeneral),
(IbcSudoChange, Group::UnbundleableSudo),
// TODO: should these have a different group?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an answer for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think BundleableGeneral is correct, as it's an action that's ok to be bundled and isn't a sudo-action (but it is a permissioned action, just not sent by the sudo address)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the other option is to make a new BundleableMarketMapAdmin group, but not sure that's really necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of any problem in using BundleableGeneral either. Happy to not add a new group if we don't really need to :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this TODO line should be deleted pls.

@@ -30,6 +31,10 @@ message Action {
FeeAssetChange fee_asset_change = 53;
FeeChange fee_change = 55;
IbcSudoChange ibc_sudo_change = 56;

// Oracle actions are defined on 61-70
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should start this range at 71 or even higher to leave plenty of room for other sudo actions in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, doesn't hurt, i will change

Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

No blockers now - just three outstanding comments/threads left.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants