Update ic.did #75
Update ic.did #75
Conversation
✅ Deploy Preview for ic-interface-spec ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
It's hardly a spec if the implementation defines what's true; it becomes implementation then ;-) Don't we have spec acceptance tests? Shouldn't they prevent such accidental difference? It seems somewhere some shortcuts were taken… Looking at the diff it seems mostly artifacts of Rust specific naming conventions (e.g. upper case enum tags). Ideally that is not the guiding light for a language agnostic interface, and I'd say it's more important to be consistent within the interface. If all the other enums and variants have lowercase names, then these here better follow suit. |
| testnet; | ||
| Mainnet; | ||
| Testnet; | ||
| Regtest; |
There was a problem hiding this comment.
FYI, https://developer.bitcoin.org/examples/testing.html#regtest-mode
For local bitcoin development, regtest is the chain being connected. The relevant PR here.
There was a problem hiding this comment.
Regtest is a tool that we're choosing to add to facilitate local development, but it's not part of production, so IMO it does not belong to the interface spec.
| @@ -52,8 +52,8 @@ type get_utxos_request = record { | |||
| address : bitcoin_address; | |||
| network: bitcoin_network; | |||
| filter: opt variant { | |||
| min_confirmations: nat32; | |||
| page: blob; | |||
| MinConfirmations: nat32; | |||
| transform : opt variant { | ||
| function: func (http_response) -> (http_response) query | ||
| }; | ||
| transform_method_name : opt text; |
There was a problem hiding this comment.
Why are we changing this ? I will be on the side of using as much as type safety as possible .
There was a problem hiding this comment.
Indeed, this is not clear, could someone clarify?
There was a problem hiding this comment.
We need to analyze whether both Motoko and Rust can support this. What constraints does this impose on the use of potential other languages like C?
There was a problem hiding this comment.
@nomeata : Can you help in clarifying whether and how one would implement in Rust against the current specification (lines 118-120)? Does it preclude other languages with a less expressive type system like C to be used for developing canisters?
I would really like to keep the spec as in 118-120. Thanks for any input!
There was a problem hiding this comment.
Motoko has no problem supporting function references. On the Rust side, you can decode it to https://docs.rs/candid/0.7.16/candid/types/reference/struct.Func.html. But there is a problem in encoding the reference type. There are some workaround implemented in ic-proxy, but not ideal. See this issue: dfinity/candid#273
There was a problem hiding this comment.
BTW, I think we are debating exactly the same points as when we debated whether to use a virtual canister for management, or use system API. I had a hard time convincing Johan of the virtual canister approach, and only @ielashi managed to frame it in a way that convinced him. Maybe Islam can help coming to a conclusion here too :-)
There was a problem hiding this comment.
I am feeling the pressure now :-) I'm catching up here, so apologies in advance if I'm missing some major points.
I think we're all in agreement that we should keep the low level system API as minimal as possible, and the dilemma here is whether the transform method is a primitive that needs to be added to the system API, or whether it can be expressed using other primitives.
If I understand correctly, what makes the transform special, as Dimitris already mentioned, is the following:
- There's no sender (and hence it's somewhat different from a query).
- It should (ideally) not be invoked by any canister apart from the canister itself and the management canister.
IMHO, the first property isn't really necessary, and the second property can be enforced with other lower-level primitives.
Regarding "there's no sender", why is that the case? The canister is requesting from the management canister to do an HTTP request, and the management canister then calls the transform method, so it seems perfectly consistent to have the management canister as the sender. Are there any reasons we need this special casing?
As for access control, which is more of a nice-to-have than a hard requirement, I believe we're also all in agreement that we should charge for queries at some point, and with that we'll need some mechanism to allow canisters to accept/reject processing queries (i.e. some kind of equivalent to the canister_inspect_message we have for ingress messages). Once that primitive is available, then a canister can easily ensure that the transform method can only be called by itself and/or the management canister if it wishes.
With the above in mind, I'd be inclined to think the transform method is really no different than a query. What would we gain by special-casing this method and introducing a new system API?
As for the debate of whether to use a func or a text for the transform method, I don't have a strong opinion. I agree that func feels a bit too generic, but on the other hand I like the additional type safety that it can provide with the right support from a langauge's CDK, so I'd have a slight preference for that.
There was a problem hiding this comment.
I feel there's still a misunderstanding here on what we mean when we say the management canister "calls" the transform function.
The design is that the replica calls the transform function and not really the management canister. If it was an actual call from the management canister, I'd expect it to be similar to an inter-canister call but the feature is designed in a way that the call does not resemble an inter-canister call. Perhaps it could have been designed and implemented in such a way but I simply don't see it in the current design and implementation.
That said, I feel we're just going in circles here. This discussion was brought up internally and a few people (tagged here and including myself) found the system method approach to be matching better. It seems there's another set of people that feel differently and we probably need to agree we disagree. Each side has presented their arguments, so I propose we leave it up to @Dfinity-Bjoern and other spec owners (and ultimately PR approvers) to set the direction.
There was a problem hiding this comment.
The design is that the replica calls the transform function and not really the management canister.
That sentence doesn’t make sense to me. The management canister isn’t a think, it is an abstract concept, a facade for the replica (or the subnet, if you want). Just like if a canister calls the management canister to get some randomness, of course what’s happening really is the replica handles that.
But yes, I think the arguments are on the table. Poor @Dfinity-Bjoern will have to make a call :-)
There was a problem hiding this comment.
I must admit @nomeata swayed me to the let-it-be-a-query direction. The main reason reason is that, from a canister perspective, the API of calling aaaaa-aa and then getting a "callback" (in quotes since it's not a callback in the interface spec sense) from aaaaa-aa makes a lot of sense. It is, in a sense, upward-compatible to providing the functionality through an actual canister. So if I have to make a call, then we go with:
- we leave it as a query that is invoked (with caller
aaaaa-aa), - we also keep the type-safe variant.
| @@ -19,7 +18,7 @@ type definite_canister_settings = record { | |||
| type http_header = record { name: text; value: text }; | |||
|
|
|||
| type http_response = record { | |||
| status: nat; | |||
| status: nat64; | |||
There was a problem hiding this comment.
The team thinks that this should be changed to nat64 in the spec as it does not make sense to have an arbitrary precision number for representing HTTP status codes.
There was a problem hiding this comment.
Probably not that important, but this goes against the general advice by @rossberg to not use fixed-width numbers unless you do bit-fiddling or need to be compatible with some crypto or it’s a fixed width hash or something like that.
Also note that nat is actually shorter on the wire than nat64 for small values.
And even if, why nat64 and not nat16 or nat32?
There was a problem hiding this comment.
I wonder if this push to use nat64 (here and elsewhere) is mostly an artifact of the Rust bindings being designed in a way that makes the use of nat more inconvenient than nat64.
This could probably be fixed by adding a newtype around i64 to the Rust library that encodes/decodes to nat, and errors out when the incoming number is too large. Then Rust code can interact with nat with very little extra work whenever it wants to reject very large values anyways. WDYT, @chenyan-dfinity?
There was a problem hiding this comment.
i128/u128 already maps to int/nat in Candid. The question is whether we want to optimize space for messages or the Rust memory.
We could define a newtype for i64, but people may still choose to ignore it. Just like we already have candid::Int and candid::Nat defined. Maybe we need an option to map all Rust number types to nat/int on the wire?
There was a problem hiding this comment.
@nomeata : Thanks for your thoughts about this and the reference to Andreas Rossberg's opinion. So you would advise to keep it as a nat?
There was a problem hiding this comment.
Yes, I guess just use i128 in rust and not worry any further :-)
There was a problem hiding this comment.
@nomeata, @chenyan-dfinity :Sounds good! Thanks for your helpful inputs here!
| @@ -112,12 +112,10 @@ service ic : { | |||
| http_request : (record { | |||
| url : text; | |||
| max_response_bytes: opt nat64; | |||
| method : variant { get; head; post }; | |||
| http_method : variant { GET; HEAD; POST }; | |||
There was a problem hiding this comment.
We will change the enumerations to lowercase in the implementation.
However, we wonder whether using "method" or "http_method" is better, considering that it is part of a record that has "http" in its name.
There was a problem hiding this comment.
I have a slight preference for just calling it "method" since it's in the context of an http_request and the HTTP spec refers to it as "request method" (not "HTTP method"). But I don't think it matters too much, I find it clear either way.
There was a problem hiding this comment.
Agreed, than let's go with
method : variant { get; head; post };
as it is currently in the spec and change the implementation.
|
Regarding the Bitcoin API, we discussed it internally and we agree that consistency of the interface is more important, and we'll be updating the implementation to comply with the spec. |
|
Note that the bitcoin interface in the replica has been updated to be consistent with the current spec, so the changes proposed to the Bitcoin interface in this PR can be reverted. |
|
In short, we all agree to keep the existing specification candid and let the teams fix the implementation to be consistent. |
Some types of
bitcoinandcanister_http_requesthave different definition in the IC implementation.Should we update the candid file in the spec or change the actual implementation?
Also delete
user_idsince it is not used by any methods.