-
Notifications
You must be signed in to change notification settings - Fork 267
feat(lazer): improve metadata in protobuf #2856
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
optional FeedState state = 107; | ||
|
||
|
||
// [required] Feed status in the current shard. Disabled feeds will not be visible in |
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.
sorry i got a bit lost here with feedState. what cases does it cover that feed state is not :? is it like we add a same feed to all the shards that we need this?
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.
Feed state is a consumer-facing and publisher-facing property that's also used to limit access to the feeds. is_enabled_in_shard
is a purely internal thing for moving live feeds between shards.
} | ||
|
||
impl From<&DynamicValue> for ProtobufDynamicValue { | ||
fn from(value: &DynamicValue) -> Self { |
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 think generally the expectation from From
is to consume the object.
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.
Well, it consumes the object, but the object is a reference. I don't think it's particularly unusual. If you look at std impls, there is a bunch of implementations on references. I think it's reasonable to impl From
for references when conversion from a reference is more efficient than clone()
followed by a conversion. In this case we can avoid cloning Vec
and BTreeMap
unnecessarily.
STABLE = 1; | ||
// Inactive feeds are not available to consumers or publishers. |
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 is technically available to publishers now.
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.
What do you mean by "technically available"? Can you suggest a proper description of this state?
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 think he means publishers can publish to them regardless of the state of the feed? We should change this in the future though.
Perhaps you can specify they are feeds which can/can't be subscribed to?
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.
// Inactive feeds are not available to consumers or publishers. | |
// Inactive feeds are not available to consumers. |
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 think the only difference in INACTIVE and COMING_SOON is to inform the end user that inactive feed will never become active.
optional string market_schedule = 106; | ||
// [required] Feed state | ||
optional FeedState state = 107; | ||
|
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 think we should add asset_type here too. Because we will be using asset_type to determine what kind of feed type is expected (e.g funding-rate vs regular price).
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 call it feed type.
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.
@ali-behjati are you saying it's different from asset_type and we have both? Because I was thinking we could infer feed type based on asset_type. and multiple asset_types can have the same feed type.
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 also prefer to have feed type (Price/FundingRate) because those options are what we care about in the implementation. (It's already like this in the transaction protobuf). Asset type has a lot of options and I think it's better to leave it in the dynamic metadata.
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 called it FeedKind
because type
is annoying to use in Rust 😅
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.
LGTM but will let Keyvan/Ali give final approval since they left their own comments already. Just a handful of small questions.
message AddFeed { | ||
// [required] ID of the feed. Must be unique (within the shard). | ||
// [required] |
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.
Why was this description removed? Is it no longer accurate?
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 a feed ID needs to change as a result of moving to another shard which already has that ID (further, how do we prevent this from happening?), how would that impact subscribing to the feed? Is this ID separate from the customer/publisher facing feed ID?
// IDs of publishers enabled for this feed. | ||
repeated uint32 permissioned_publishers = 3; | ||
// [required] | ||
optional DynamicValue.Map metadata = 3; |
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.
Any reason we skip field ID 2 if this is not in use yet?
STABLE = 1; | ||
// Inactive feeds are not available to consumers or publishers. |
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 think he means publishers can publish to them regardless of the state of the feed? We should change this in the future though.
Perhaps you can specify they are feeds which can/can't be subscribed to?
// Feed kind determines the set of data fields available in the feed. | ||
// It also determines the kind of data accepted from publishers for this feed | ||
// (`PriceUpdate` or `FundingRateUpdate`). | ||
enum FeedKind { |
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 intended to introduce this in some form. I will use the state enum to guard rail what updates a feed is able to accept.
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.
we should use this for guard railing updates
// multiple shards, it must only be active in one of them at each time. | ||
// To enforce this, `pending_activation` and `pending_deactivation` fields | ||
// To enforce this, `enable_in_shard_timestamp` and `disable_in_shard_timestamp` fields |
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.
To confirm my understanding, Aggregator pulls all shards. So, it is able to handle this switch in logic?
Does this break the current agent transactions? |
UpdateFeedProperties update_feed_properties = 101; | ||
UpdateFeedMetadata update_feed_metadata = 102; | ||
EnableFeedInShard enable_feed_in_shard = 103; | ||
DisableFeedInShard disable_feed_in_shard = 104; | ||
google.protobuf.Empty remove_feed = 199; |
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.
we probably don't need this, since we will make feeds inactive instead of deleting.
Summary
This PR splits old content of
FeedMetadata
into two categories:name, exponent, min_publishers, min_rate, expiry_time, market_schedule, state
. These are now properties of theFeed
message.symbol, description, asset_type, quote_currency
), and for matching with external systems (cmc_id, hermes_id
). These fields may be used by publishers and by history service (for feed search API), but are otherwise treated as opaque in the Lazer core system. They are now stored as a collection of dynamically typed key-value pairs. This gives the operations team more freedom to extend and change metadata.Other relevant changes in this PR:
is_activated
tois_enabled_in_shard
(same for other related fields) to clarify the meaning and intended usage, and to avoid confusion withFeed.state
.DynamicValue
Rust type to deal with metadata without loss of information.Rationale
For more flexible metadata.
How has this been tested?