feat: switch the client for alerts from rpc to grpc for new kava version #72
Conversation
boodyvo
requested review from
drklee3,
pirtleshell,
DracoLi,
lbayas,
sesheffield and
evgeniy-scherbina
| // Create codec for messages | ||
| encodingConfig := kava.MakeEncodingConfig() | ||
|
|
||
| pruningGRPCClient, err := kavagrpc.NewClient("https://grpc.kava.io:443") |
There was a problem hiding this comment.
Ty for writing tests!
In general, I would prefer not to have tests depend on live infra, as it's a bit brittle. However, I'll take it over no tests! I think the best approach would be to use mocks and store the example responses as files.
There was a problem hiding this comment.
I saw such type of tests in other repos and like that because it helps to validate that it will work on mainnet at least. Yeh, agree that it is brittle as depends on external things, but we always can test it live. We can put it behind "non short" or something like that.
Alternatively, I can implement the some kind of VCR or other stubs to substitute server (not sure, if it works for grpc).
| @@ -0,0 +1,21 @@ | |||
| package auctions | |||
There was a problem hiding this comment.
can we add a readme to this folder?
Auction alerts tracks ongoing auctions on the Kava Chain and alerts for:
- Total value of auctions above configured value
- Percentage price deviation of auction clearing price above configured value
- Price deviation of USDX below configured value
There was a problem hiding this comment.
Added to the module README.md as those alerts you described are actually 2 different services (one for auctions and one for usdx)
| require.Len(t, markets, 10) | ||
| } | ||
|
|
||
| func TestGrpcGetMoneyMarkets(t *testing.T) { |
There was a problem hiding this comment.
This test seems to be failing - should have 29 item(s), but has 16
There's currently no CI workflow that runs these tests, would be great to add one
There was a problem hiding this comment.
Added. The thing I noticed is that auction-audit test failed. At the moment removed them from CI (to not block this one) and investigating what happen there
alerts/auctions/grpc_client.go
Outdated
|
|
||
| // GrpcAuctionClient defines a client for interacting with auctions via rpc | ||
| type GrpcAuctionClient struct { | ||
| conn *grpc.ClientConn |
There was a problem hiding this comment.
unused field
| conn *grpc.ClientConn |
There was a problem hiding this comment.
Strange that it was not caught up by the linter. Should we add flags to set it up to catch such misses?
There was a problem hiding this comment.
Updated a lint, but still it doesn't catch such cases 😱 And we have lots of lint errors with hard-keeper-bot, so added at the moment that as a TODO for lint and update and created ticket for that (as not directly related to this update): https://app.shortcut.com/kava-labs/story/14431
There was a problem hiding this comment.
Great! Yeah definitely room for improvement with the CI - ideally we have a set of consistent workflows for all repos that need it, following the main ones we use in Kava repo
There was a problem hiding this comment.
This one I picked up from kava repo, as I remember we discussed them and reviewed appropriately.
No description provided.