Query result does not contain query name

grim-i-am commented 2 years ago

What Git revision are you using?

v5.3.0 binary

What operating system are you using, and which version?

[ ] Linux / Ubuntu
[X] Linux / Other
[ ] OSX
[ ] Windows

Describe what the problem is?

When handling a Query response it is not possible to determine which type of query the result is for.

What should be the expected behavior?

It would be nice if the query name is also included in the Query result.

KtorZ commented 2 years ago

Yes indeed, this is something I've been thinking about for a while. In the end, since the server guaranties ordering of requests / responses already and, because of the mirror/reflection (which one can use to do just that) it has been put on the stack of "things I might do if someone requests it" :sweat_smile:

KtorZ commented 2 years ago

One of the main reason I didn't do it so far is that, if we stay close to the assumed semantic of the JSON/WSP specification, the "request" is a "Query" and the response is therefore for that query. It's arguments of the query that tells the type of query, but from the PoV of the envelope, it's just a Query.

Having said that, it feels like the envelope is more working against than helping us. This whole WSP thingy is something I meant to drop, possibly in the next major version of Ogmios. And here, it'll be a good time to also implement this kind of feature.

grim-i-am commented 2 years ago

the "request" is a "Query" and the response is therefore for that query

that would work with one off requests but if you're firing off many requests inside a centralised service then it becomes more difficult to match the query with the result. Not impossible, but difficult. One could store a request reference in a FIFO queue and pop the next available reference once a result is received. Another way indeed would be to use the mirror argument. However I find that this would just add to the already inconsistent way of handling results. And this introduces the need for more processing.

As I've mentioned before, I really like this tool. However there are a couple things that I believe could be more consistent.

For example:

For Method calls we receive the method name back as a property value that we can use a switch statement on. This is good.
For certain methods, the result is inside a key that we need to test for, like RollBackward/Forward, so we need to use an if/else statement
For RollForward we need to extract the key name for the era to be able to access the block data:
```
"block":
{ "alonzo":
  { "body":
     [...        
```
If it were as follows, then it would be easier to access body without having to check the name of the property
```
"block":
{ era: "alonzo"
, "body":
  [...        
```
For Query results, we need to track the order of the queries or provide a mirror.
For evaluateTx and submitTx, the parameter name for the transaction data is different (evaluate/submit) I would just have it as "tx" or something for both of them.

So different features of the library require different strategies to handle them.

These might seem like little, trivial things. However the more consistently structured the various parts of a system are, the more patterns can be identified. This allows a solution to be applied at a higher level of abstraction, leading to a cleaner and more concise implementation with less code.

For example, these two function of mine, having a different parameter key evaluate and submit:

function evaluateTx(networkId, cborHex) {
    return new Promise((resolve, reject) => {
        let reqId = getUniqueId();

        networks[networkId].requests[reqId] = { resolve: resolve, reject: reject };

        wsp(Methods.evaluateTx, { evaluate: cborHex}, { networkId: networkId, reqId: reqId });
    });
}

function submitTx(networkId, cborHex) {
    return new Promise((resolve, reject) => {
        let reqId = getUniqueId();

        networks[networkId].requests[reqId] = { resolve: resolve, reject: reject };

        wsp(Methods.submitTx, { submit: cborHex}, { networkId: networkId, reqId: reqId });
    });
}

could be combined into one if they had a common tx parameter key:

function evaluateOrSubmitTx(methodName, networkId, cborHex) {
    return new Promise((resolve, reject) => {
        let reqId = getUniqueId();

        networks[networkId].requests[reqId] = { resolve: resolve, reject: reject };

        wsp(methodName, { tx: cborHex}, { networkId: networkId, reqId: reqId });
    });
}

Just my two cents that will hopefully be taken as positive criticism.

KtorZ commented 2 years ago

I mostly agree with your remarks regarding the API, in particular the way some results are currently wrapped behind keys that make them a bit harder to digest. This is not a problem in every languages, but few people have raised this already. Yet, as for Query, I can't change anything without breaking the API and thus, rolling a new major version. I would rather not do that before the next hard-fork. So that at least, people get time to integrate the hard-fork changes before moving to the new major.

Regarding evaluateTx / submitTx, this is a conscious design decision to precisely discourage people from writing something like evaluateOrSubmitTx and shoot themselves in the foot too easily. Note however that, even with the current approach, you can still define the function in a relatively straightforward way :no_mouth:

function evaluateOrSubmitTx(methodName, networkId, cborHex) {
    return new Promise((resolve, reject) => {
        let reqId = getUniqueId();

        networks[networkId].requests[reqId] = { resolve: resolve, reject: reject };

        const key = methodName.slice(0, -2).toLowerCase();

        wsp(methodName, { [key]: cborHex}, { networkId: networkId, reqId: reqId });
    });
}

I've parked this ticket with the changes coming with v6.0.0; this a bigger overhaul of the API planned with that major version to fix inconsistency and easier to digest. So, I appreciate your feedback and will welcome more :pray:

grim-i-am commented 2 years ago

this is a conscious design decision to precisely discourage people from writing something like evaluateOrSubmitTx and shoot themselves in the foot too easily.

Such mistakes would be discovered immediately in development upon the first test.

I can't change anything without breaking the API

Yeah, I understand. That's a problem.

{ [key]: cborHex}

TIL! thanks (Although this nullifies your conscious design decision :stuck_out_tongue_winking_eye: )

Thanks for the exchange. Always a pleasure!

CardanoSolutions / ogmios