feat: removal message #772
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
=======================================
Coverage 57.01% 57.01%
=======================================
Files 208 210 +2
Lines 11626 11718 +92
=======================================
+ Hits 6628 6681 +53
- Misses 4394 4429 +35
- Partials 604 608 +4 ☔ View full report in Codecov by Sentry. |
aljo242
requested review from
Eric-Warehime,
technicallyty,
wesl-ee and
zrbecker
as code owners
Co-authored-by: Tyler <[email protected]>
proto/connect/marketmap/v2/tx.proto
Outdated
|
|
||
| // MsgRemoveMarketsResponse defines the | ||
| // Msg/MsgRemoveMarketsResponse response type. | ||
| message MsgRemoveMarketsResponse {} |
There was a problem hiding this comment.
I assume we are only succeeded if all markets to be removed exist, and are successfully removed which is why we don't need a response, maybe.
We could support making this message idempotent where we instead succeed if either the market does not exist, or was successfully removed. In that case it might make sense to return how many markets were actually removed.
There was a problem hiding this comment.
Updated to return []string which is the array of markets that were successfully removed.
x/marketmap/keeper/keeper.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if market.Ticker.Enabled { |
There was a problem hiding this comment.
There is a bit of asymmetry here since we typically check for Enabled markets for updating in the ante handlers, but here are making it part of policy for all integrations.
Perhaps we should call this method "DeleteMarket", and write something like,
for _, handler := k.deleteMarketHandlers {
if err := handler(ctx, market); err != nil {
return err
}
}
return k.markets.Remove(ctx, types.TickerString(tickerStr))
We can define a DefaultDeleteMarketHandlers() somewhere and use that to initialize the keeper.
Not sure if handler, verifier, validator is better.
This also ensures if we want to add more complex logic later for a specific integration, we can do so without changing connect.
There was a problem hiding this comment.
Added the notion of ValidationHooks which can be configured for the keeper.
Right now just passing them via options at instantiation which keeps the NewKeeper function signature the same
| @@ -14,6 +14,9 @@ type MarketMapHooks interface { | |||
|
|
|||
| // AfterMarketGenesis is called after x/marketmap init genesis. | |||
| AfterMarketGenesis(ctx sdk.Context, tickers map[string]Market) error | |||
|
|
|||
| // AfterMarketRemoved is called after a market is removed. | |||
| AfterMarketRemoved(ctx sdk.Context, market Market) error | |||
There was a problem hiding this comment.
If this hook errors, does the message fail?
Could we check if the removed market was enabled here, and just fail?
There was a problem hiding this comment.
Yes if these hooks fail, the message would fail
Closes CON-1749
ValidationHooksOptionsconfiguration to keeper instantiation