Skip to content
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

[Access] Change SendAndSubscribe endpoint's MessageIndex to start at 0 #6575

Closed
Tracked by #6163
peterargue opened this issue Oct 18, 2024 · 1 comment
Closed
Tracked by #6163

Comments

@peterargue
Copy link
Contributor

Problem Definition

The SendAndSubscribeTransactionStatuses endpoints includes a MessageIndex field to help the client check that they received all messages. Currently, this value starts at 1 instead of 0, which is a little confusing since almost all iteration fields start at 0, and it's documented that it starts at 0.
https://github.com/onflow/flow/blob/0b0946dbb2683cec184522b0e3e88246b076a2dc/protobuf/flow/access/access.proto#L753-L754

Other endpoints are documented to start at 1, but actually start at 0:
https://github.com/onflow/flow/blob/0b0946dbb2683cec184522b0e3e88246b076a2dc/protobuf/flow/executiondata/executiondata.proto#L469-L470
https://github.com/onflow/flow/blob/0b0946dbb2683cec184522b0e3e88246b076a2dc/protobuf/flow/executiondata/executiondata.proto#L621-L622

Proposed Solution

Let's update the logic and docs to use 0 as the first value for all endpoints.

This is the current logic for SendAndSubscribeTransactionStatuses

flow-go/access/handler.go

Lines 1435 to 1450 in 0dde730

messageIndex := counters.NewMonotonousCounter(0)
return subscription.HandleSubscription(sub, func(txResults []*TransactionResult) error {
for i := range txResults {
value := messageIndex.Increment()
err = stream.Send(&access.SendAndSubscribeTransactionStatusesResponse{
TransactionResults: TransactionResultToMessage(txResults[i]),
MessageIndex: value,
})
if err != nil {
return rpc.ConvertError(err, "could not send response", codes.Internal)
}
}
return nil
})

We could update it to be similar to the logic in other endpoints

index := messageIndex.Value()
if ok := messageIndex.Set(index + 1); !ok {
return status.Errorf(codes.Internal, "message index already incremented to %d", messageIndex.Value())
}
err = send(&executiondata.SubscribeAccountStatusesResponse{
BlockId: convert.IdentifierToMessage(resp.BlockID),
BlockHeight: resp.Height,
Results: results,
MessageIndex: index,
})

@illia-malachyn
Copy link
Contributor

If you run new examples onflow/flow-go-sdk#794 via make stream-account-statuses, you will run into an error msg received out of order. The current implementation expects it to start from 0, but it actually starts from 1 even though the tests expect it to start from zero. So, this is the bug and it has to be fixed asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants