[Access] Add script execution to Access API #4791
Conversation
| var coded fvmerrors.CodedError | ||
| if fvmerrors.As(err, &coded) { | ||
| // general FVM/ledger errors | ||
| if coded.Code().IsFailure() { | ||
| return rpc.ConvertError(err, "failed to execute script", codes.Internal) | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is correct for detecting a cadence error locally.
Currently, we assume any error from fvm is "InvalidArgument":
flow-go/engine/execution/rpc/engine.go
Lines 194 to 198 in 259992f
it seems like we could do better. I'm ok just leaving the logic the same for now though.
peterargue
force-pushed
the
petera/4781-integrate-local-script-exec
branch
from
55dea45
to
c249e94
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4791 +/- ##
==========================================
+ Coverage 55.57% 55.85% +0.27%
==========================================
Files 872 944 +72
Lines 82036 87767 +5731
==========================================
+ Hits 45591 49019 +3428
- Misses 32930 35058 +2128
- Partials 3515 3690 +175
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| if err != nil { | ||
| b.log.Error().Err(err).Msgf("failed to get account at blockID: %v", latestBlockID) | ||
| b.log.Debug().Err(err).Msgf("failed to get account at blockID: %v", sealedBlockID) |
There was a problem hiding this comment.
this could fail for a lot of reasons that aren't errors occurring on the node, so I figure it's better to produce fewer error level messages in that case.
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to convert account message: %v", err) | ||
| if errors.Is(err, ErrDataNotAvailable) { |
There was a problem hiding this comment.
can use convertScriptExecutionError here too?
There was a problem hiding this comment.
not directly since it has some additional error type handling done in convertScriptExecutionError that doesn't apply to getting accounts, but I could extract the common bits out to a separate function
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to convert account message: %v", err) | ||
| if errors.Is(err, ErrDataNotAvailable) { | ||
| return nil, status.Errorf(codes.OutOfRange, "data for block height %d is not available", height) |
There was a problem hiding this comment.
how does it work that when the height is for past sporks, we query historical AN?
There was a problem hiding this comment.
no, it works the same as it does today, except instead of a state commitment not found error, we return data for block height x is not available. The client will need to first check if the height they are querying for is within the current spork
Co-authored-by: Gregor G. <75445744+sideninja@users.noreply.github.com>
|
|
||
| // get the latest sealed header | ||
| latestHeader, err := b.state.Sealed().Head() | ||
| sealed, err := b.state.Sealed().Head() |
There was a problem hiding this comment.
just want to double-check, this state used here is the same state as used by script executor right?
There was a problem hiding this comment.
I don't think the script executor uses state directly, just headers. but they are both populated by the follower engine
| @@ -760,7 +759,7 @@ func (builder *FlowAccessNodeBuilder) BuildExecutionSyncComponents() *FlowAccess | |||
| return nil, err | |||
| } | |||
|
|
|||
| builder.ScriptExecutor = scripts | |||
| builder.ScriptExecutor.InitReporter(builder.ExecutionIndexer, scripts) | |||
There was a problem hiding this comment.
Is it possible that we don't create the script executor until its dependencies are ready, this eliminates the case where ScriptExecutor's method is called before the indexer and scripts model are passed in.
There was a problem hiding this comment.
The challenge is that we want the API to run before the checkpoint loads. So there needs to be some way for the API to check if scripts is available. I used the approach of wrapping it in engine/access/rpc/backend/script_executor.go, but am open to other approaches if you have other ideas.
There was a problem hiding this comment.
+1 I've used the same approach for GetRegisterValues
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Closes: #4781
Add local script execution to access API