-
Notifications
You must be signed in to change notification settings - Fork 180
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] Add implementation for transaction statuses data providers #6818
base: master
Are you sure you want to change the base?
[Access] Add implementation for transaction statuses data providers #6818
Conversation
…The-K-R-O-K/flow-go into AndriiDiachuk/6586-tx-statuses-data-providers-impl
…' of github.com:The-K-R-O-K/flow-go into AndriiDiachuk/6586-tx-statuses-data-providers-impl
…github.com:The-K-R-O-K/flow-go into AndriiDiachuk/6586-tx-statuses-data-providers-impl
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6818 +/- ##
==========================================
+ Coverage 41.10% 41.14% +0.04%
==========================================
Files 2107 2109 +2
Lines 185286 185571 +285
==========================================
+ Hits 76156 76358 +202
- Misses 102727 102797 +70
- Partials 6403 6416 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
engine/access/rest/websockets/data_providers/transaction_statuses_provider_test.go
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/transaction_statuses_provider.go
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/transaction_statuses_provider.go
Outdated
Show resolved
Hide resolved
…The-K-R-O-K/flow-go into AndriiDiachuk/6586-tx-statuses-data-providers-impl
return nil, status.Errorf(codes.Internal, "message index already incremented to %d", messageIndex.Value()) | ||
} | ||
|
||
return &models.TransactionStatusesResponse{ |
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.
@UlyanaAndrukhiv this is just a note not to forget to include this PR to #6802, or should @AndriiDiachuk make changes in this PR?
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.
@Guitarheroua, yes responses will be included in #6802.
engine/access/rest/websockets/data_providers/send_transaction_statuses_provider_test.go
Show resolved
Hide resolved
} | ||
|
||
return &models.TransactionStatusesResponse{ | ||
TransactionResults: txResults, |
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.
we should return results one by one, not as an array, check handler.go
implementation as an example:
Lines 1435 to 1454 in 941178d
messageIndex := counters.NewMonotonousCounter(0) | |
return subscription.HandleRPCSubscription(sub, func(txResults []*TransactionResult) error { | |
for i := range txResults { | |
index := messageIndex.Value() | |
if ok := messageIndex.Set(index + 1); !ok { | |
return status.Errorf(codes.Internal, "message index already incremented to %d", messageIndex.Value()) | |
} | |
err = stream.Send(&access.SendAndSubscribeTransactionStatusesResponse{ | |
TransactionResults: TransactionResultToMessage(txResults[i]), | |
MessageIndex: index, | |
}) | |
if err != nil { | |
return rpc.ConvertError(err, "could not send response", codes.Internal) | |
} | |
} | |
return nil | |
}) |
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.
The same should be done for TransactionStatusesDataProvider
…github.com:The-K-R-O-K/flow-go into AndriiDiachuk/6586-tx-statuses-data-providers-impl
|
||
// TransactionStatusesResponse is the response message for 'events' topic. | ||
type TransactionStatusesResponse struct { | ||
TransactionResults []*access.TransactionResult `json:"transaction_results"` |
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.
TransactionResults []*access.TransactionResult `json:"transaction_results"` | |
TransactionResult *access.TransactionResult `json:"transaction_result"` |
Closes: #6586
In this PR
TransactionStatusesDataProvider
andSendTransactionStatusesDataProvider
was implemented.New functionality was covered with tests.