-
Notifications
You must be signed in to change notification settings - Fork 9
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
QuickJSPlugin: read external data #33
base: main
Are you sure you want to change the base?
Conversation
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "warp-contracts-plugin-quickjs", | |||
"version": "1.1.12", |
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.
?
async readExternal(processIdHandle: QuickJSHandle, actionHandle: QuickJSHandle) { | ||
const processId = this.vm.getString(processIdHandle); | ||
const action = this.vm.getString(actionHandle); | ||
const { dryrun } = await import('@permaweb/aoconnect'); |
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.
why dynamic import?
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.
already discussed
tags: [{ name: 'Action', value: action }], | ||
data: '1234' | ||
}); | ||
return JSON.parse(readRes.Messages[0].Data); |
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.
maybe there is no point in parsing the response if we need to stringify it again to pass it to the quickjs..
quickJsEvaluator.evalLogging(); | ||
quickJsEvaluator.evalPngJS(); | ||
quickJsEvaluator.evalRedStone(); | ||
quickJsEvaluator.evalExternal(); |
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.
shouldn't evalExternal
and dummyPromiseEval
also be called only if isSourceAsync
?
@@ -23,7 +23,7 @@ export const vmIntrinsics = { | |||
...DefaultIntrinsics, | |||
Date: false, | |||
Proxy: false, | |||
Promise: false, | |||
Promise: true, |
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.
shouldn't we set this dynamically based on isSourceAsync
?
@@ -8,14 +8,18 @@ export class QuickJsHandlerApi<State> { | |||
constructor( | |||
private readonly vm: QuickJSContext, | |||
private readonly runtime: QuickJSRuntime, | |||
private readonly quickJS: QuickJSWASMModule, |
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.
I guess the QuickJSWASMModule import can also now be removed?
if (state) { | ||
this.initState(state); | ||
} | ||
return this.runContractFunction(message, env); | ||
return await this.runContractFunction(message, env); |
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.
technically await is not needed here
const result = this.vm.evalCode(`(async () => { | ||
return await __handleDecorator(${JSON.stringify(message)}, ${JSON.stringify(env)}) | ||
})()`); | ||
const promiseHandle = this.vm.unwrapResult(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.
how are erros how handled? e.g. what if dry-run timeouts?
evalExternal() { | ||
const readExternalHandle = this.vm.newFunction('readExternal', (processIdHandle, actionHandle) => { | ||
const promise = this.vm.newPromise(); | ||
this.readExternal(processIdHandle, actionHandle).then((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.
shouldn't we also somehow handle errors here and pass some error info to the quickjs?
const processId = this.vm.getString(processIdHandle); | ||
const action = this.vm.getString(actionHandle); | ||
const { dryrun } = await import('@permaweb/aoconnect'); | ||
const readRes = await dryrun({ |
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.
what is the default timeout for the dryrun? maybe we should it limit to sth. like 10s?
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.
i dont know what's the default but i've added some logic so the error is thrown if dryrun is executing more than 10s
}); | ||
return JSON.parse(readRes.Messages[0].Data); | ||
try { | ||
const result = await Promise.race<{ |
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 issue with race is that the 'timeout' promise won't be removed even if the 'raced' promise is settled.
https://stackoverflow.com/a/39852372
It is already handled by a timeout implementation in warp sdk - https://github.com/warp-contracts/warp/blob/main/src/utils/utils.ts#L52
No description provided.