Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add service initialization parameters #88

Merged
merged 4 commits into from
Sep 15, 2020
Merged

add service initialization parameters #88

merged 4 commits into from
Sep 15, 2020

Conversation

chenyan-dfinity
Copy link
Contributor

Discussed in dfinity/motoko#1549. This adds initialization parameters to the higher order case as well. Since this requires changing the binary format, better to change it now than later.

Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to say something in the serialisation and value sections as well? Even if it is just that references uninitialised services are not supported at the moment.

spec/Candid.md Outdated Show resolved Hide resolved
spec/Candid.md Outdated
@@ -144,10 +144,12 @@ A *service* is a standalone actor on the platform that can communicate with othe

#### Structure

A service's signature is described by an *actor type*, which defines the list of *methods* that the service provides. Each method is described by its *name* and a *function type* describing its signature. The function type can also be given by referring to a type definition naming a function reference type.
A service's signature is described by an *actor type*, which defines the list of *methods* that the service provides.
A service can optionally provide *initialization parameters* for creating services dynamically with the same signature.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it may be good to make the difference more explicit. The parameters aren't just "optional". If they are present, the type describes a different kind of entity. That probably deserves a separate paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if { <methtype>;* } should just be a sugar for () -> { <methtype>;* }? So the presence of parameters represents different stages of the service: uninitialized vs installed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what context? In general, they are totally different things, the former typing a canister reference, the latter a Wasm module. They are no more related than Nat and () -> Nat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the sense that the did file generated by Motoko is only a Wasm module. It becomes a canister only after installation. So the actor from Motoko is always () -> service.

spec/Candid.md Outdated Show resolved Hide resolved
spec/Candid.md Outdated

```
<actortype> ::= { <methtype>;* }
<actortype> ::= { <methtype>;* } | <tuptype> -> { <methtype>;* }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also allow forming

... | <tuptype> -> <id>

where <id> refers to an initialised actor type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we? It's symmetric with functions, which don't allow <tuptype> -> <id>. What does (nat) -> service (int) -> { f: () -> () } mean? Initializing this service gets another uninitialized service?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't suggest that you can nest them, thus initialised actor type. Only that you can write this:

type A = service {...};
type B = service (Nat) -> A;

The id can only be a non-arrow type, i.e.,

type C = service (Nat) -> B;

would be an error.

chenyan-dfinity and others added 2 commits August 25, 2020 10:45
Co-authored-by: Andreas Rossberg <rossberg@dfinity.org>
@crusso
Copy link
Contributor

crusso commented Aug 26, 2020

Hmm, I don't see why we are going higher-order now, when it is unnecessary and won't be supported for a while. What was wrong with the original proposal from the discussion:

<prog>  ::= <def>;* <actor>;?
<def>   ::= type <id> = <datatype> | import <text>
<actor> ::= service <id>? : (<tuptype> ->?)  (<actortype> | <id>)

without extending <actortype> itself.

(dfinity/motoko#1549 (comment))

This doesn't require introducing (first-class) types for classes (nor any wire format) and just adds the possibility of parameterising the main service.

@chenyan-dfinity
Copy link
Contributor Author

I don't see why we are going higher-order now, when it is unnecessary and won't be supported for a while.

Because the higher order case is a breaking change for wire format. Better to change the format sooner? Otherwise, we can just extend the main actor.

@crusso
Copy link
Contributor

crusso commented Aug 27, 2020

I don't see why we are going higher-order now, when it is unnecessary and won't be supported for a while.

Because the higher order case is a breaking change for wire format. Better to change the format sooner? Otherwise, we can just extend the main actor.

Fair enough, maybe that should be @rossberg's call.

@rossberg
Copy link
Contributor

I'm fine "reserving" the syntax and wire format already. I'm also fine restricting for now. I'm not yet concerned about adding extensions to the format that are technically breaking changes. Ones we're live that becomes more of an issue.

What would it actually take to support the higher-order case? The wire format of a service constructor would just be the blob containing the Wasm module, wouldn't it? If that's all then it seems pretty straightforward.

@crusso
Copy link
Contributor

crusso commented Aug 27, 2020

I'm fine "reserving" the syntax and wire format already. I'm also fine restricting for now. I'm not yet concerned about adding extensions to the format that are technically breaking changes. Ones we're live that becomes more of an issue.

What would it actually take to support the higher-order case? The wire format of a service constructor would just be the blob containing the Wasm module, wouldn't it? If that's all then it seems pretty straightforward.

Yeah, I'm not sure about that. Ideally you'd probably just want it to be the id/address of some installed code, not the code itself, but who knows. The current model of literally passing the code around and duplicating at each instantiation seems unfortunate to me, though it's the only method of instantiation we have at the moment. To my mind, installing a service module should install the code once and return a reference to a constructor that peels off fresh instances sharing the code, but providing new instance data/state.

@rossberg
Copy link
Contributor

Yeah, I agree that it would be nicer if the platform had a way to reference modules, though I doubt we'll see much excitement for that from the execution team. The easiest way to get there might be by providing an inefficient version of the feature and wait for performance pressure from actual use cases. :) Then we can still extend the wire format to support both ways of passing it.

@chenyan-dfinity
Copy link
Contributor Author

If breaking changes are not a concern for now, I would prefer to restrict the change to main actor for now. I don't fully understand the higher order case, as it is tied to the question of where we store the candid file. PTAL.

@chenyan-dfinity
Copy link
Contributor Author

Do we want to merge this?

@nomeata
Copy link
Collaborator

nomeata commented Sep 3, 2020

I guess. I wasn’t part of the discussion the last two weeks, so maybe @crusso can make that call. But the PR as it stands seems innocent enough.

@chenyan-dfinity chenyan-dfinity merged commit a6ea099 into master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants