Add execute endpoint #101
Add execute endpoint #101
Conversation
tweag-ev-ak
force-pushed
the
add_execute_endpoint
branch
from
db644b3 to
ed1b378
Compare
| case evalPwcExUnitsWithLogs pwc exMaxBudget of | ||
| Right (pwc', logs, exUnits) -> pure (pwc', exUnits, logs, Nothing) | ||
| Left (ValidationFailure exUnits evalErr logs pwc') -> pure (pwc', exUnits, logs, Just evalErr) | ||
| _err -> fail "The script evaluation has failed" |
There was a problem hiding this comment.
Which case is this matching? We aren't using MonadFail much, and I think that we should at least tell the user why it failed if we have more info.
There was a problem hiding this comment.
Yeah, the simple show err doesn't work here because of the era constraints, I postponed it until the next PRs.
lib/PSR/ContextBuilder.hs
Outdated
| case C.toScriptInEra (C.convert ctxAlonzoEraOnwards) rsScriptFileContent of | ||
| Nothing -> Nothing | ||
| Just scriptInEra -> Just (C.toShelleyScript scriptInEra) |
There was a problem hiding this comment.
Isn't this just C.toShelleyScript <$> C.toScriptInEra (C.convert ctxAlonzoEraOnwards) rsScriptFileContent?
There was a problem hiding this comment.
Yep, a leftover after copy-paste editing.
| , rsScriptEvaluationParameters :: ScriptEvaluationParameters | ||
| , rsScriptForEvaluation :: ScriptForEvaluation |
There was a problem hiding this comment.
These can be removed now (removed in my prometheus PR)
tweag-ev-ak
force-pushed
the
add_execute_endpoint
branch
from
ed1b378 to
8231f60
Compare
tweag-ev-ak
force-pushed
the
add_execute_endpoint
branch
from
8231f60 to
b855f76
Compare
adithyaov
left a comment
adithyaov
left a comment
There was a problem hiding this comment.
Few question but overall LGTM.
We might want to take a look at error handling again later.
| , evalError :: Maybe EvalError | ||
| , exUnits :: ExUnits | ||
| , context :: ExecutionContext | ||
| , context :: Either ExecutionContext ExecutionContextId |
There was a problem hiding this comment.
Is my understanding correct: The reason for this change is to provide an ExecutionContextId instead of the whole ExecutionContext while returning the result for /execute.
Is there any other use case in mind?
Can't we just return the entire ExecutionContext? We can just leave this as ExecutionContext?
There was a problem hiding this comment.
Ah, looks like this also serves another purpose. It differentiates manual execution from automatic execution.
A followup question: Do we want the event stream to contain manual executions?
| Just rs -> do | ||
| (_, exUnits, logs, evalError') <- tryRunScriptInContext rs context | ||
| liftIO $ | ||
| events.addExecutionEvent blockHeader $ |
There was a problem hiding this comment.
/execute is not a read + execute.
It's read + execute + write.
This modifies the database and adds and event to the event stream.
Is this what is intended?
There was a problem hiding this comment.
That's a good question and yeah, I thought about making an option so it's possible to execute without writing.
This PR and #103 are iterations to this point.
| deriving (Show, Eq) | ||
|
|
||
| -- resolveScript :: ScriptInAnyLang -> _ | ||
| resolveScript :: C.ScriptInAnyLang -> ExceptT String IO (ScriptEvaluationParameters, ScriptForEvaluation) |
There was a problem hiding this comment.
This was removed earlier in another PR IIRC.
| , rsScriptEvaluationParameters | ||
| , rsScriptForEvaluation |
There was a problem hiding this comment.
I think we can remove this.
May have been added unintentionally.
There was a problem hiding this comment.
We use to run the scripts in the /execute endpoint
| , -- TODO: Should this be incorporated into rsScourse? | ||
| -- Can't be Just when rsSource is Nothing | ||
| rsScriptFileContent :: Maybe C.ScriptInAnyLang | ||
| , rsScriptFileContent :: C.ScriptInAnyLang |
There was a problem hiding this comment.
It was not required but should be.
closes #86
After this PR we can re-organize the events flow.