-
Notifications
You must be signed in to change notification settings - Fork 61
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: Reduce blast radius of errors in MarketMap #736
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
+ Coverage 55.52% 55.58% +0.06%
==========================================
Files 208 208
Lines 11586 11613 +27
==========================================
+ Hits 6433 6455 +22
- Misses 4554 4557 +3
- Partials 599 601 +2 ☔ View full report in Codecov by Sentry. |
x/marketmap/types/market.go
Outdated
// | ||
// In particular, this will eliminate anything which would otherwise cause a failure in ValidateBasic. | ||
// The resulting MarketMap should be able to pass ValidateBasic. | ||
func (mm *MarketMap) GetValidSubset() MarketMap { |
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 feel like this function should return an error
as well and call Validate
to ensure that the returned map is valid
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.
There is a case where there is no valid subset and we should just return nil, error
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 you're right--if the entire set of markets is invalid we should just keep everything the same. Returning an error will do that while I think returning an empty subset would just stop fetching any prices at all.
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.
otherwise lgtm
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 pending @aljo242's suggestion
… (#850) Co-authored-by: Eric Warehime <eric.warehime@gmail.com>
Adds a method to the marketmap type,
GetValidSubset
. This is used in the update lifecycle in order to be able to accept updates to the marketmap which do not fully pass ValidateBasic.It will attempt to remove any
ProviderConfig
with an invalidNormalizeByPair
, and then remove anyMarket
which fails to passValidateBasic
.If a market has some valid ProviderConfigs it can still run as long as it still satisfies the min provider count.