Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

feat: redefine data source protos #514

Closed
wants to merge 1 commit into from

Conversation

AnExsomnis
Copy link
Contributor

@AnExsomnis AnExsomnis commented Jul 20, 2022

close vegaprotocol/vega#5714

This PR redefines the proto types related to oracles following the logic presented in vegaprotocol/specs#1161

changes introduced:

  • For all external data sources - we care about pubkeys and filters -> so we apply same properties for all (this follows the logic in feat: redefine data sourcing concepts. specs#1161)
    => Everywhere where we have oracle definitions we use the generic data source type instead of custom oracle type

  • There is an option where we can embed generic data type into the oracle type - if we really need specific properties for oracles. Same of course can be applied further for different groups of oracles (ETH, or not), bridges as a separate group of external sources, whatever we would need.

The idea behind the changes is:

  • use a way as straight forward as possible (doing the correct thing is not always the simplest change)
  • do not involve the design with any workaround -> if a workaround is needed - isolate it from the design, so when it needs to be removed - it will not harm the idea.
  • have generic types on early stage (like now) ad build up on them if you have to

@AnExsomnis AnExsomnis added the enhancement New feature or request label Jul 20, 2022
@AnExsomnis AnExsomnis requested a review from a team as a code owner July 20, 2022 13:58
@AnExsomnis AnExsomnis self-assigned this Jul 20, 2022
@AnExsomnis AnExsomnis requested a review from barnabee July 20, 2022 14:06
@gordsport gordsport added this to the 🤠 Oregon Trail milestone Jul 20, 2022
@gordsport gordsport requested review from davidsiska-vega and a team July 26, 2022 08:46
Comment on lines 30 to 31
DataSpecConfiguration oracle = 1;
DataSpecConfiguration bridge = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get what these 2 are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a oneof setting for different type of external spec configuration - we can define an oracle or a bridge.
But this was added in order to show how different external types would be treated anyway, so I'll just leave oracle for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the unused one

Copy link
Member

@jeremyletang jeremyletang left a comment

Choose a reason for hiding this comment

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

Just the commands might need updating relative to these changes.

@AnExsomnis
Copy link
Contributor Author

Just the commands might need updating relative to these changes.

@jeremyletang , Addressed all of your comments and rebased with develop

@jeremyletang
Copy link
Member

Those changes looks good to me @AnExsomnis, let's move them over onto the vega repo on a new PR please. thanks.

@gordsport
Copy link
Contributor

closing as done in:

@gordsport gordsport closed this Nov 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request no-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up oracle spec declaration on market
4 participants