-
Notifications
You must be signed in to change notification settings - Fork 179
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 usage of the local transaction result in Access API #5306
[Access] Add implementation for usage of the local transaction result in Access API #5306
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5306 +/- ##
==========================================
+ Coverage 56.06% 57.42% +1.35%
==========================================
Files 1026 798 -228
Lines 100116 80463 -19653
==========================================
- Hits 56129 46205 -9924
+ Misses 39688 30565 -9123
+ Partials 4299 3693 -606
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
…eroua/4753-local-index-tx-result
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.
thanks for the PR @Guitarheroua! it's all coming together.
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
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.
Great job, very big lift. Most of my comments are stylistic and around documentation.
The only thing I am unsure is Initialize
, I would prefer to avoid such patterns but if they are absolutely needed then they need to be carefully explained. Additionally I don't think case of "not initialized" should be threaten as sentinel error to me it seems that we require initialization before we can serve clients, in other words our invariant is: "Initialize MUST to be called before serving clients" if this invariant is broken then we should bail out with exception, not a sentinel error.
// get the latest finalized block from the state | ||
finalized, err := t.state.Final().Head() | ||
if err != nil { | ||
return flow.TransactionStatusUnknown, irrecoverable.NewExceptionf("failed to lookup final header: %w", err) |
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.
👍
Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>
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.
Proposed a few more godoc changes, otherwise looks good!
return reporter.HighestIndexedHeight() | ||
} | ||
|
||
func (t *TransactionResultsIndex) checkDataAvailability(height uint64) error { |
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 need some godoc for this function, don't forget to describe errors
func (suite *Suite) checkTransactionResultResponse( | ||
err error, | ||
response *acc.TransactionResult, | ||
block flow.Block, | ||
txId flow.Identifier, | ||
txFailed bool, | ||
eventsForTx []flow.Event, |
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.
func (suite *Suite) checkTransactionResultResponse( | |
err error, | |
response *acc.TransactionResult, | |
block flow.Block, | |
txId flow.Identifier, | |
txFailed bool, | |
eventsForTx []flow.Event, | |
func (suite *Suite) assertTransactionResultResponse( | |
err error, | |
response *acc.TransactionResult, | |
expectedBlock flow.Block, | |
expectedTxId flow.Identifier, | |
expectedTxFailed bool, | |
expectedEvents []flow.Event, |
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.
Proposed a few more godoc changes, otherwise looks good!
Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>
…dex-tx-result [Access] Add implementation for usage of the local transaction result in Access API
#4753
Add implementation for usage of the local transaction result index for existing Access API endpoints(
GetTransactionResult
,GetTransactionResultByIndex
,GetTransactionResultsByBlockID
) to get transaction results for indexed blocks.The errors for transaction results are requested separately and were implemented as part of the issue #4754.