Code refactoring in `Desmo` module

iosonopersia commented 2 years ago

[ ] Do we really need two separate functions to execute a query?

Why cannot we merge together buyQuery and getQueryResult into an atomic function called executeQuery?

In this case, we wouldn't need to store the dealid into the this.dealid member... Here there's an example of why the current code can be a source of problems:

const desmo = new Desmo(...);

await desmo.buyQuery(...); // this.dealid <-- DEAL_A
await desmo.buyQuery(...); // this.dealid <-- DEAL_B

// The following function call sees this.dealid == DEAL_B,
// hence it returns RESULT_B instead of RESULT_A:
const resultA = await desmo.getQueryResult(...);

[x] Also, this is what we return when executing a query (see #12):
```
{
requestID: string;
taskID: string;
result: number | string;
}
```

Why cannot we simply return the result? Are the other values really useful for users of the SDK?

[ ] What's the purpose of the verifyCallbackAddress method? Do we really need it in the Desmo module?

relu91 commented 2 years ago

Do we really need two separate functions to execute a query?

I have to investigate the role of dealId before answering. I am puzzled as I was here. Having dealID saved as an instance variable seems like a bad design to me too. But I have to dig further. @iotondato?

Also, this is what we return when executing a query

I think we returned those IDs for easy accessing and referring to the transaction ID from the frontend.

What's the purpose of the verifyCallbackAddress method? Do we really need it in the Desmo module?

It should be a sanity check that the DApp is not compromised but I need another pass of iExec documentation reading to be sure if it is really needed.

iosonopersia commented 2 years ago

I have to investigate the role of dealId before answering. I am puzzled as I was https://github.com/vaimee/desmo-sdk/pull/1#discussion_r944791903. Having dealID saved as an instance variable seems like a bad design to me too. But I have to dig further. @iotondato?

I may have found a partial answer to why we select the first appOrder and the first workerpoolOrder. I strongly believe that relevant code was taken by @iotondato from this demo during the initial stages of the SDK development. Have a look at file src/index.js , lines 154 to 165...

Unluckily, this doesn't explain why it's done in the first place, maybe is just a simplification which is OK for a demo but not for production (or maybe is OK for production also, I have no clue about this).

I think we returned those IDs for easy accessing and referring to the transaction ID from the frontend.

Then we'll leave it as it is.

vaimee / desmo-sdk

Code refactoring in `Desmo` module #12