Add ability to make an actor from an interface#79
Conversation
| makeActorInterface: ({ IDL }: { IDL: typeof _IDL }) => _IDL.ActorInterface, | ||
| ) => ( | ||
| httpAgent: HttpAgent, | ||
| ): Record<string, (...args: Array<any>) => any> => { |
There was a problem hiding this comment.
This may not be obvious so I want to draw attention to it.
The way this currently works is that makeActorInterface is the JS generated by the IDL compiler. It may also be written by hand as is done in the tests.
At present, makeActorInterface is a function that takes the IDL object exported from IDL.js. The motivation for this is described here: caffeinelabs/motoko#693
I think the important thing to note here is that what the IDL compiler produces aren't really "bindings". They weren't actually bindings before I made that PR, but arguably they had the potential to be because they imported the user library (sort of, see the PR linked above).
Now the user library imports what the IDL compiler generates, which is a JS representation of the actor's interface. I think this makes a lot more sense given the information required to communicate with a client, such as canister IDs, entity IDs, etc
There was a problem hiding this comment.
Thanks for the context. Yes, I am missing some related context here, so will continue in the morning.
|
Perhaps @chenyan-dfinity should take a look also. I will continue tomorrow. The first pass looks fine. I need to ensure I understand the actor module. |
| decode(x: Buffer): JsValue | ||
| } | ||
|
|
||
| export interface Text extends Type {} |
There was a problem hiding this comment.
I don't know enough JS, but why do we want to export Text, not other IDL types?
There was a problem hiding this comment.
Good point. I just added the minimal definitions to get a demo working.
| }; | ||
|
|
||
| const expectedReplyArg = new Uint8Array( | ||
| _IDL.Text.encode("Hello, World!").buffer, |
There was a problem hiding this comment.
What's the difference between _IDL.Text and IDL.Text?
There was a problem hiding this comment.
They're the same thing. On line 13 IDL is imported as _IDL because TypeScript got confused about line 19 where IDL was both a field in a record and a type.
| test("makeActor", async () => { | ||
| const actorInterface = ({ IDL }: { IDL: typeof _IDL }) => { | ||
| return new IDL.ActorInterface({ | ||
| greet: IDL.Func([IDL.Text], [IDL.Text]), |
There was a problem hiding this comment.
@chenyan-dfinity above is the format we used in the produce exchange, but I see the following format used in the IDL tests:
greet: IDL.Func(IDL.Obj({ "0": Text }), IDL.Obj({ "0": Text })),
Are these supposed to be equivalent? If so I think it has implications for encoding and decoding, when we get to that.
There was a problem hiding this comment.
Ah, I will fix the IDL codegen. This is a hack with the old system API.
js-user-library/src/actor.ts
Outdated
| return [methodName, async (...args: Array<any>) => { | ||
| // TODO | ||
| // * Throw if func.argTypes.length !== args.length | ||
| // * Encode arguments |
There was a problem hiding this comment.
| // * Encode arguments | |
| // * Encode arguments with the corresponding types |
| _IDL.Text.encode("Hello, World!").buffer, | ||
| ) as BinaryBlob; | ||
|
|
||
| const mockFetch: jest.Mock = jest.fn() |
There was a problem hiding this comment.
For future PR: let's try to add a test where we handle errors.
eftychis
left a comment
eftychis
left a comment
There was a problem hiding this comment.
LGTM: Just two comments to be added. Pretty good!!! Thanks, Paul 😎
3 participants
Extracted from #56, with some changes.
There are one or two points I would like to call out for discussion.