Add subscribe account statuses endpoint #762
Conversation
illia-malachyn
requested review from
janezpodhostnik,
sideninja,
chasefleming,
nvdtf and
jribbink
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #762 +/- ##
==========================================
+ Coverage 52.92% 54.44% +1.52%
==========================================
Files 35 35
Lines 5580 5780 +200
==========================================
+ Hits 2953 3147 +194
+ Misses 2231 2194 -37
- Partials 396 439 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
|
|
||
| func checkAndIncrementMessageIndex(previousMessageIndex *atomic.Uint64, currentMessageIndex uint64) error { |
There was a problem hiding this comment.
@peterargue is this implementation good enough? It might be good to add more tests for different msg indexes. However, I'm not sure what cases to cover. What do you think?
There was a problem hiding this comment.
I think you can simply it a bit by tracking "next" instead of the current.
e.g.
next := 0 // since we start at 0, the first expected should be 0
for {
if msg.MessageIndex != next {
return err
}
next = msg.MessageIndex + 1
}
| return accountStatutesChan, errChan, nil | ||
| } | ||
|
|
||
| func receiveAccountStatusesFromClient[Client interface { |
There was a problem hiding this comment.
using Client as the name is a little confusing, especially within a client library. how about breaking this out to a stand along type since it's used in a bunch of places, and renaming it to Stream?
| client Client, | ||
| accountStatutesChan chan<- flow.AccountStatus, | ||
| errChan chan<- error, | ||
| previousMsgIndex *atomic.Uint64, |
There was a problem hiding this comment.
message index is specific to a stream. So when you start a new stream, the responses start with message index 0 (or 1, I don't remember how it started), then increment.
I think it's fine to track it only within this function as a regular uint64 value. There should not be any concurrent access.
| } | ||
| } | ||
|
|
||
| func checkAndIncrementMessageIndex(previousMessageIndex *atomic.Uint64, currentMessageIndex uint64) error { |
There was a problem hiding this comment.
I think you can simply it a bit by tracking "next" instead of the current.
e.g.
next := 0 // since we start at 0, the first expected should be 0
for {
if msg.MessageIndex != next {
return err
}
next = msg.MessageIndex + 1
}| } | ||
|
|
||
| type AccountStatusResult struct { | ||
| Address Identifier |
There was a problem hiding this comment.
shouldn't this be an address?
| Address Identifier | |
| Address Address |
Closes: #747