feat(filter)!: return error codes instead of throwing errors #1971

danisharora099 · 2024-04-23T11:58:49Z

Problem

The Filter implementation currently throws Error to handle non-success scenarios.
This needs a change to using error codes instead: #1694

Solution

Move all throws to returning error codes

Notes

Related to feat: prefer error code for req-res protocol over exception #1694

github-actions · 2024-04-23T12:25:26Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku node	181.24 KB (+0.02% 🔺)	3.7 s (+0.02% 🔺)	13.7 s (+10.02% 🔺)	17.3 s
Waku Simple Light Node	181.32 KB (+0.06% 🔺)	3.7 s (+0.06% 🔺)	10.9 s (-52.21% 🔽)	14.5 s
ECIES encryption	23.12 KB (+0.19% 🔺)	463 ms (+0.19% 🔺)	4.5 s (-9.39% 🔽)	4.9 s
Symmetric encryption	22.53 KB (-0.1% 🔽)	451 ms (-0.1% 🔽)	4 s (-6.95% 🔽)	4.4 s
DNS discovery	72.5 KB (+0.11% 🔺)	1.5 s (+0.11% 🔺)	7.4 s (-51.72% 🔽)	8.9 s
Peer Exchange discovery	74.09 KB (-0.02% 🔽)	1.5 s (-0.02% 🔽)	9.2 s (-3.59% 🔽)	10.7 s
Local Peer Cache Discovery	67.63 KB (-0.03% 🔽)	1.4 s (-0.03% 🔽)	9.1 s (-39.91% 🔽)	10.5 s
Privacy preserving protocols	38.88 KB (+0.04% 🔺)	778 ms (+0.04% 🔺)	8.3 s (-14.03% 🔽)	9.1 s
Waku Filter	111.72 KB (-0.07% 🔽)	2.3 s (-0.07% 🔽)	22.5 s (+72.79% 🔺)	24.8 s
Waku LightPush	110.26 KB (+0.1% 🔺)	2.3 s (+0.1% 🔺)	16.7 s (+6.03% 🔺)	18.9 s
History retrieval protocols	110.76 KB (+0.02% 🔺)	2.3 s (+0.02% 🔺)	22.3 s (+143.7% 🔺)	24.5 s
Deterministic Message Hashing	7.24 KB (-0.15% 🔽)	145 ms (-0.15% 🔽)	2.3 s (+129.65% 🔺)	2.4 s

adklempner · 2024-04-24T06:31:03Z

packages/sdk/src/protocols/filter.ts

+        return {
+          failures: [
+            {
+              error: ProtocolError.TOPIC_DECODER_MISMATCH


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

adklempner · 2024-04-24T06:45:05Z

packages/sdk/src/protocols/filter.ts

+              error: ProtocolError.TOPIC_DECODER_MISMATCH
+            }
+          ],
+          successes: []


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:

the state here, when there is a single error in failures and no item in successes, which is like a "total failure"

some items in successes and some items in failures, which means some subscriptions were successful

some items in successes and no items in failures, which means all subscriptions were successful

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

packages/interfaces/src/misc.ts

weboko

We need to postpone this PR in favor of #1958

weboko · 2024-04-26T08:28:46Z

packages/core/src/lib/filter/index.ts

+      return {
+        success: null,
+        failure: {
+          error: ProtocolError.GENERIC_FAIL,


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

…-error-codes

weboko · 2024-04-30T10:59:43Z

packages/core/src/lib/filter/index.ts

        `Filter unsubscribe all request ${requestId} failed with status code ${statusCode}: ${statusDesc}`
      );
+      return {
+        failure: {
+          error: ProtocolError.REMOTE_PEER_REJECTED,


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

packages/sdk/src/protocols/filter.ts

packages/interfaces/src/filter.ts

weboko · 2024-04-30T22:32:11Z

packages/sdk/src/protocols/filter.ts

@@ -381,7 +418,7 @@ class FilterSDK extends BaseProtocolSDK implements IFilterSDK {
    return toAsyncIterator(this, decoders);
  }

-  private getPubsubTopics<T extends IDecodedMessage>(
+  private getUniquePubsubTopics<T extends IDecodedMessage>(


why unique? it just returns pubsubTopics without duplicates

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

packages/core/src/lib/filter/index.ts

weboko · 2024-05-07T09:34:42Z

packages/core/src/lib/filter/index.ts

-      );
+      return {
+        failure: {
+          error: ProtocolError.REMOTE_PEER_FAULT,


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

weboko · 2024-05-07T09:40:23Z

packages/tests/tests/filter/ping.node.spec.ts

-      subscription = await waku.filter.createSubscription(TestShardInfo);
+      const { error, subscription: _subscription } =
+        await waku.filter.createSubscription(TestShardInfo);
+      if (!error) subscription = _subscription;


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

done, thanks! also updated in other tests

danisharora099 added 7 commits April 23, 2024 12:53

move protocol result type to interfaces

dd1ce6d

chore: update type names for verbosity

8226ee3

feat(filter-core): convert error throws to return types

2303e79

chore: update types & imports

07ed52f

update Filter API

f03e86d

chore: update createSubscription

b6c116e

chore: update imports & rename

2688ec9

chore: update all tests

cf9fda2

danisharora099 marked this pull request as ready for review April 23, 2024 12:45

danisharora099 requested a review from a team as a code owner April 23, 2024 12:45

adklempner reviewed Apr 24, 2024

View reviewed changes

danisharora099 requested review from a team and adklempner April 25, 2024 12:01

adklempner approved these changes Apr 25, 2024

View reviewed changes

weboko requested changes Apr 26, 2024

View reviewed changes

weboko reviewed Apr 26, 2024

View reviewed changes

danisharora099 added 6 commits April 29, 2024 19:51

chore: resolve conflicts & merge (1/2)

05b1baa

chore: resolve conflicts & merge (2/n)

318bbfd

chore: resolve conflicts & merge (3/n)

85cb393

chore: resolve conflicts & merge (4/n)

ffb9b2d

chore: resolve conflicts & merge (5/n)

891ed66

chore: resolve conflicts & merge (6/n)

fc1cacf

danisharora099 requested a review from weboko April 30, 2024 10:47

danisharora099 self-assigned this Apr 30, 2024

Merge branch 'master' of github.com:waku-org/js-waku into feat/filter…

4e37405

…-error-codes

weboko reviewed Apr 30, 2024

View reviewed changes

packages/sdk/src/protocols/filter.ts Outdated Show resolved Hide resolved

weboko reviewed Apr 30, 2024

View reviewed changes

packages/interfaces/src/filter.ts Show resolved Hide resolved

weboko reviewed Apr 30, 2024

View reviewed changes

weboko mentioned this pull request May 2, 2024

feat(filter)!: limit filter subscriptions to use a single decoder #1981

Closed

chore: use idiomatic approach

f2d6d8f

danisharora099 mentioned this pull request May 7, 2024

chore: remove the suffix SDK from user-facing protocol types #1998

Open

chore: fix tests

f48c58d

weboko reviewed May 7, 2024

View reviewed changes

packages/core/src/lib/filter/index.ts Outdated Show resolved Hide resolved

weboko reviewed May 7, 2024

View reviewed changes

weboko self-requested a review May 7, 2024 09:41

danisharora099 and others added 3 commits May 7, 2024 19:26

chore: address comments

fd7d61c

chore: fix test

0f7d3c2

Merge branch 'master' into feat/filter-error-codes

1f6e420

weboko approved these changes May 9, 2024

View reviewed changes

rm: only

365fae0

danisharora099 merged commit 4eb06c6 into master May 9, 2024
10 checks passed

danisharora099 deleted the feat/filter-error-codes branch May 9, 2024 11:21

weboko mentioned this pull request May 9, 2024

chore: release master #2004

Merged

danisharora099 mentioned this pull request Mar 18, 2024

feat: prefer error code for req-res protocol over exception #1694

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(filter)!: return error codes instead of throwing errors #1971

feat(filter)!: return error codes instead of throwing errors #1971

danisharora099 commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading

adklempner Apr 24, 2024

danisharora099 Apr 25, 2024

adklempner Apr 25, 2024

adklempner Apr 25, 2024

danisharora099 Apr 30, 2024

adklempner Apr 24, 2024

danisharora099 Apr 25, 2024

adklempner Apr 25, 2024

weboko left a comment

weboko Apr 26, 2024

weboko Apr 26, 2024

danisharora099 Apr 30, 2024

weboko Apr 30, 2024

danisharora099 Apr 30, 2024

danisharora099 Apr 30, 2024

weboko Apr 30, 2024

danisharora099 May 7, 2024

weboko May 7, 2024

weboko Apr 30, 2024

danisharora099 May 7, 2024

weboko Apr 30, 2024

danisharora099 May 1, 2024

weboko May 7, 2024

danisharora099 May 7, 2024

weboko May 7, 2024

danisharora099 May 7, 2024

feat(filter)!: return error codes instead of throwing errors #1971

feat(filter)!: return error codes instead of throwing errors #1971

Conversation

danisharora099 commented Apr 23, 2024 • edited Loading

Problem

Solution

Notes

github-actions bot commented Apr 23, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weboko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danisharora099 commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading