Conversation
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
I wish I had more to say as a result from all the time I just spent reviewing this PR, but honestly it looks great. Well done.
| } | ||
|
|
||
| export enum RequestStatusResponseStatus { | ||
| Pending = "pending", |
There was a problem hiding this comment.
There was a problem hiding this comment.
@stanleygjones what do you recommend we do here? I originally based everything on the spec and then realize that the client was behind, and it's been challenging to know when things have changed.
js-user-library/src/callRequest.ts
Outdated
| import { SubmitRequestType } from "./submitRequestType"; | ||
|
|
||
| // The fields in a "call" submit request. | ||
| export interface CallRequest extends Request { |
There was a problem hiding this comment.
when we extend Request is there a standard way of ordering the fields in the encoding?
There was a problem hiding this comment.
Do you mean the CBOR encoding? I haven't given that much thought and have been relying on the borc package for that. It seems to be deterministic though.
| import { QueryRequest } from "./queryRequest"; | ||
| import { RequestStatusRequest } from "./requestStatusRequest"; | ||
|
|
||
| export type ReadRequest |
There was a problem hiding this comment.
The public spec has a more complex structure I realized on Friday.
There was a problem hiding this comment.
I'm pretty sure I closely followed the version I was working with at the time. Is there something in particular you can point out?
There was a problem hiding this comment.
Not necessary for this PR, just a mental note -- they are broken down in sync and async calls, certified, etc, instead of a flatter structure. We are missing the account balance and update canister requests, which are not applicable.
| // bug in the client when the same request is submitted more than once: | ||
| // https://dfinity.atlassian.net/browse/DFN-895 | ||
| nonce?: BinaryBlob; | ||
| sender_pubkey: BinaryBlob; |
There was a problem hiding this comment.
I separated out the common fields and the auth fields because some functions ignore the auth fields.
Having all fields in the same type required me to provide values to functions just for them to be immediately removed again.
| } | ||
|
|
||
| export type Request | ||
| = AuthFields |
js-user-library/src/request.ts
Outdated
| nonce?: BinaryBlob; | ||
| sender_pubkey: BinaryBlob; | ||
| sender_sig: BinaryBlob; | ||
| nonce?: Nonce; |
There was a problem hiding this comment.
So the spec states entity_id is not always part of the common field. But I think it should be part of the request always, even if it has to move to the auth fields forever.
There was a problem hiding this comment.
I hadn't realized, but the spec does say that request_type is the only field that is common to all requests, with the following being common among all asynchronous requests:
sender(EntityId)expirytime(?)nonce(blob, optional)
I think I should reflect this in the types.
There was a problem hiding this comment.
I haven't looked into how sender (entity ID) is intended to be used enough to understand your comment about putting it in every request.
Are you proposing a change to the spec? Without that, even if we send it other implementations may not, and the client won't be able to rely on it being there.
There was a problem hiding this comment.
Yes, I am proposing a change. I am trying to make sense of it though too.
|
|
||
| window.crypto = require("@trust/webcrypto"); | ||
| window.TextEncoder = require("text-encoding").TextEncoder; | ||
| require("whatwg-fetch"); |
There was a problem hiding this comment.
The existing comment at the top of the file applies to this to; the browsers we're targeting will have the fetch API, but JSDom (the test environment) does not.
| config: Config, | ||
| ) => async ( | ||
| request: ReadRequest, | ||
| ): Promise<Response> => { |
There was a problem hiding this comment.
can we timeout with promise?
There was a problem hiding this comment.
Yes, a promise could throw when a timeout is reached. Awaiting them requires wrapping in a try/catch block or using .catch to handle that.
2 participants
This adds the HTTP agent from #56, along with some refactoring. It's intended to be the lower-level interface to the HTTP API, and not the mechanism by which a typical developer interacts (at least directly) with an internet computer client.
I would like to better describe what is going on here but I need to go offline now, and wanted to at least make a PR before that.
At the moment it's a subset of what is described in the public spec. I hope the inline comments and tests can be informative until I can follow up here.