fix: EventResult should await OK response

[iFergal]

In places, we were still passing a Promise<Response> to EventResult. This is problematic because if a non 2xx code is returned as the fetch call will reject after the function has returned.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.64%. Comparing base (f368351) to head (4b1aa6d).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #299      +/-   ##
==========================================
+ Coverage   83.63%   83.64%   +0.01%     
==========================================
  Files          48       48              
  Lines        4235     4238       +3     
  Branches     1055     1055              
==========================================
+ Hits         3542     3545       +3     
  Misses        663      663              
  Partials       30       30              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lenkan]

LGTM, although, it would be nice to implement a test case that verifies this. For example by setting up a unit test with a fetch call that will return a rejected promise.

// aiding.test.ts

    it('Should throw error if fetch call fails', async () => {
        const error = new Error(`Fail ${randomUUID()}`);
        client.fetch.mockRejectedValue(error);
        await expect(
            client
                .identifiers()
                .create('aid1', { bran: '0123456789abcdefghijk' })
        ).rejects.toThrow(error);
    });
    

Currently, it believe the library will NOT throw for non-OK responses. This test will not pass:

   it('Should throw HTTP if fetch resolves with non-JSON', async () => {
       client.fetch.mockResolvedValue(
           new Response('Not JSON', { status: 500 })
       );

       await expect(
           client
               .identifiers()
               .create('aid1', { bran: '0123456789abcdefghijk' })
       ).rejects.toThrow('Fail');
   });

This is because the library does not check the HTTP status code. It will not fail until we try to read the message body as JSON, which we do in EventResult.op() method.

[iFergal]

@lenkan I'd argue this is now covered by the typing. But sure, I can add a test in case a regression happens on the types.

[iFergal]

@lenkan I didn't quite get your example. The call will throw an error if the response is e.g. 400 before checking EventResult.op().

https://github.com/WebOfTrust/signify-ts/blob/main/src/keri/app/clienting.ts#L206-L215

        const res = await fetch(this.url + path, {
            method: method,
            body: _body,
            headers: final_headers,
        });
        if (!res.ok) {
            const error = await res.text();
            const message = `HTTP ${method} ${path} - ${res.status} ${res.statusText} - ${error}`;
            throw new Error(message);
        }

[lenkan]

@lenkan I didn't quite get your example. The call will throw an error if the response is e.g. 400 before checking EventResult.op().

https://github.com/WebOfTrust/signify-ts/blob/main/src/keri/app/clienting.ts#L206-L215

Ah yes, sorry. I saw that and was just about to write a comment about this. I was mistaken. I thought it didn't, but that is because the SignifyClient.fetch is mocked in the test.

[lenkan]

Let me know if you want to add anything more, otherwise I can merge it.

[iFergal]

@lenkan Thanks, I added the first test for completeness. Nothing else to add now from my side.