When passing multiple decoders, this does not tell the user which decoder is mismatched.

It's not about which decoder, but about a mismatch in general.

Example:
DecoderA: /abc/xyz
DecoderB: /qwe/rty

So here, it's not about which decoder doesn't match the other, but the binary result that either they all match, or they don't.

Wdyt? @adklempner

The comparison is made between the decoder's pubsub topic and the subscription manager's pubsub topic. So you can have a case where all the decoders match each other but do not match the pubsub topic set in the subscription manager. This can be quickly detected if we log both pubsub topics.

I think if we go with changes described in this issue then it won't be as much of a problem: #1981

i like this change

I'm concerned with how much implicit state to handle is being passed on to the library consumer. Right now the SDKProtocolResult can mean several different things:

1. the state here, when there is a single error in failures and no item in successes, which is like a "total failure"
2. some items in successes and some items in failures, which means some subscriptions were successful
3. some items in successes and no items in failures, which means all subscriptions were successful
4. no items in successes and no items in failures, which should not happen, but technically is possible according to the type

It would be better if this function could only ever return either a failure or a success. But that might mean limiting it to one decoder at a time.

It would be better if this function could only ever return either a failure or a success. But that might mean limiting it to one decoder at a time.

Correct. nwaku/protobuf currently allows us to pass multiple content topics to the same request -- limiting to one decoder (->one content topic) on js-waku API side will be limiting of that.
This seems to be an API change that we will introduce.

I agree with your overall comment of the potential mindload on library consumer, as it could be confusing to have both failure and success haha.

Not sure what the right step is.
We can track this in an issue and discuss them in the next PM call @adklempner

I created an issue for it, let me know if I captured it correctly: #1981

shouldn't it be more concrete that generic?

I think we should make errors as informative as possible (without creating a mess) as it is the only thing developer would get, so let's try not to use GENERIC_FAIL imho

shouldn't it be more concrete that generic?

no, this is also what we do during any lightpush request.

if pipe fails unexpectedly, we can't be sure of what went wrong.

is there no response that we can read? I expect full node potentially send some message if it is on the side of it, but if failure is in pipe - then it can be generic (though in that case I still think there is a reason we can root cause)

can you repro unexpected fails of pipe?

can you repro unexpected fails of pipe?

no test vector as such for that

--

this try catch case actually handles error if error is thrown and there is no defined response from the pipe operation

if you follow the next few lines you will see we also handle errors based on bad values of res

eg:

    try {
      res = await pipe(
        [query.encode()],
        lp.encode,
        stream,
        lp.decode,
        async (source) => await all(source)
      );
    } catch (err) {
      log.error("Failed to send waku light push request", err);
      return {
        success: null,
        failure: {
          error: ProtocolError.GENERIC_FAIL,
          peerId: peer.id
        }
      };
    }

    const bytes = new Uint8ArrayList();
    res.forEach((chunk) => {
      bytes.append(chunk);
    });

    let response: PushResponse | undefined;
    try {
      response = PushRpc.decode(bytes).response;
    } catch (err) {
      log.error("Failed to decode push reply", err);
      return {
        success: null,
        failure: {
          error: ProtocolError.DECODE_FAILED,
          peerId: peer.id
        }
      };
    }

    if (!response) {
      log.error("Remote peer fault: No response in PushRPC");
      return {
        success: null,
        failure: {
          error: ProtocolError.REMOTE_PEER_FAULT,
          peerId: peer.id
        }
      };
    }

    if (!response.isSuccess) {
      log.error("Remote peer rejected the message: ", response.info);
      return {
        success: null,
        failure: {
          error: ProtocolError.REMOTE_PEER_REJECTED,
          peerId: peer.id
        }
      };
    }

I believe in that case GENERIC_FAIL on pipe means - failed to send a request, as it fails locally.

I believe in that case GENERIC_FAIL on pipe means - failed to send a request, as it fails locally.

hmm, could it fail while receiving a response for the remote peer?
are we sure about that?

dunno, I think we need to investigate it - if it bothers us

here and in similar places - can we map statusCode to some tangible error

like in case if full node has no capacity (ddos etc) - it might return some particular code

to cover this I think we should do follow up, and sync with @NagyZoltanPeter of what error codes are expected here and in similar places

agree - that would be useful.

we do follow generic error handling based on statusCode:

 const { statusCode, requestId, statusDesc } =
      FilterSubscribeResponse.decode(res[0].slice());

    if (statusCode < 200 || statusCode >= 300) {
      log.error(
        `Filter unsubscribe all request ${requestId} failed with status code ${statusCode}: ${statusDesc}`
      );
      return {
        failure: {
          error: ProtocolError.REMOTE_PEER_REJECTED,
          peerId: peer.id
        },
        success: null
      };
    }

creating an issue to track this: #1999

why unique? it just returns pubsubTopics without duplicates

yes, so it only returns unique pubsub topics, right? ^.^

it's more of no response error than remote peer fault

changed that to be a GENERIC_FAIL as done for other pipe failures. we can discuss the right error code here if required

if error - let's throw from beforeEachCustom so that we see it failed to get subscription

done, thanks! also updated in other tests