[DataAvailability] Implement fork-aware Script Endpoints#7952
Conversation
…xecutionStateQuery for rest requests
…. Refactored parsing for GetScript
…t routes to support current behaviour
…ates on indexer script executor
… nodes, added mocked version for Registers in snapshot. Linted
…cutor. Generated new mocks
…ub.com:onflow/flow-go into UlianaAndrukhiv/7651-fork-aware-script-endpoints
…tReader and in related files
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
peterargue
left a comment
peterargue
left a comment
There was a problem hiding this comment.
mostly small comments. this is probably the last set of comments.
| var failure fvmerrors.CodedFailure | ||
| if fvmerrors.As(err, &failure) { | ||
| return rpc.ConvertError(err, "failed to execute script", codes.Internal) | ||
| return access.NewInternalError(err) | ||
| } |
There was a problem hiding this comment.
CodedFailures are actually exceptions.
There was a problem hiding this comment.
I’m not fully sure I follow - do you mean that fvmerrors.CodedFailure represents an irrecoverable condition rather than a normal, expected internal error? (I’m asking because of the FailureCodeLedgerFailure, which is a FailureCode)
There was a problem hiding this comment.
Yes. For instance FailureCodeLedgerFailure means that the script failed to read data from the db. since the reads within scripts do not return an error for not found, this means there was an exception.
| // A result value is returned encoded as byte array. An error will be returned if script | ||
| // doesn't successfully execute. | ||
| // Expected errors: | ||
| // - Script execution related errors |
There was a problem hiding this comment.
looking at the error handling, I think we already handle these properly for script execution. we just need to adjust the error docs
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
|
|
||
| // ExecuteScriptAtBlockID executes provided script at the provided block ID. | ||
| // | ||
| // CAUTION: this layer SIMPLIFIES the ERROR HANDLING convention |
There was a problem hiding this comment.
I don't like the fact that we repeat this wording everywhere. I understand it might be useful if you use these functions from a different module (e.g. while working on sth on execution node). However, I don't think the protocol code would ever depend on abstractions from an access node. It also feels like either: a) package/file level comment. b) contract in access node with which everyone in the team is aware of (which also could be documented in package/file level godoc)
Perhaps, we can have wth like access/doc.go with the explanation about error handling?
There was a problem hiding this comment.
I agree. This seems useful only at the API interface level, so I’ve removed it in 4319e83
There was a problem hiding this comment.
please add this back. Alex requested we include this for all endpoint methods (implementation and interface definitions).
peterargue
deleted the
UlianaAndrukhiv/7651-fork-aware-script-endpoints
branch
4 participants
Note:
Closes: #7651
Related PR:
Context
ScriptExecutorinterface and impl (according to suggestion in comment).RegisterSnapshotReaderto handle the range checks.ExecuteScriptAtLatestBlock,ExecuteScriptAtBlockHeight,ExecuteScriptAtBlockID).criteria.gRPCandRESThandlers to extract and pass criteria from requests.