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

feat: add prim call_raw : (Principal, Text, Blob) -> async Blob #3086

Merged
merged 9 commits into from
Jan 30, 2022

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Jan 29, 2022

Fixes #2703 (by lowering ourselves to the level of Rust).

Adds a prim to dynamically invoke a method by name with an already serialized blob and get the raw (undeserialized) blob back asynchronously:

call_raw : (canister : Principal, function_name : Text, arg : Blob) -> async Blob

The function can only be called in an asynchronous context and this is enforced by the type system.

There is no assumption that the contents of either blob is Candid, so this could also be used to talk to non-Candid endpoints.

This should be sufficient to implement the call-forwarding functionality of the Rust cycles wallet.

  • Determine whether the method name must be (rope)-normalized before use. Currently it is not. @nomeata, what's the representation invariant for ordinary shared functions - I see they are (Principal,Text) pairs, but is the Text normalized?
  • Any ideas for better name: request, send, call, invoke, call_dynamic spring to mind

@crusso crusso changed the title Claudio/raw call feat: add prim call_raw : (Principal, Text, Blob) -> async Blob Jan 30, 2022
@github-actions
Copy link

github-actions bot commented Jan 30, 2022

Comparing from 5f678e9 to 87fe52a:
In terms of gas, no changes are observed in 3 tests.
In terms of size, no changes are observed in 3 tests.

@crusso crusso requested review from nomeata and ggreif January 30, 2022 10:34
src/prelude/prim.mo Outdated Show resolved Hide resolved
@nomeata
Copy link
Collaborator

nomeata commented Jan 30, 2022

Determine whether the method name must be (rope)-normalized before use. Currently it is not. @nomeata, what's the representation invariant for ordinary shared functions - I see they are (Principal,Text) pairs, but is the Text normalized?

The new call_raw takes a Text, and you certainly need to make that contiguous before passing it to the system API. I'm not sure how shared functions representation play a role here?

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@nomeata
Copy link
Collaborator

nomeata commented Jan 30, 2022

Ah, I see why, because you construct such a function ref pair in the backend.

I don't think it's clearly defined what the representation is yet, presumably, so far only flat text values occurred. So the value needs to flattened at the right place…

let add_cycles = Internals.add_cycles env ae in
compile_exp_vanilla env ae p ^^
compile_exp_vanilla env ae m ^^
Tuple.from_stack env 2 ^^ set_meth_pair ^^
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you need to turn the text into a blob, because above we have

(* The method name *)
      get_meth_pair ^^ Arr.load_field 1l ^^ Blob.as_ptr_len env ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent, thanks. That's precisely what I was worried about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@crusso crusso added the automerge-squash When ready, merge (using squash) label Jan 30, 2022
@mergify mergify bot merged commit 1c4e18e into master Jan 30, 2022
@mergify mergify bot deleted the claudio/raw-call branch January 30, 2022 21:35
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 30, 2022
};

do {
let m = "super"#"cali"#"fragilisticexpialidocious";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just being curious, did you see this crashing before it started working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I tried it locally before fixing

@@ -406,3 +406,8 @@ func @create_actor_helper(wasm_module_ : Blob, arg_ : Blob) : async Principal =
});
return canister_id_;
};

// raw calls
func @call_raw(p : Principal, m : Text, a : Blob) : async Blob {
Copy link
Contributor

@ggreif ggreif Jan 30, 2022

Choose a reason for hiding this comment

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

Why not define this in prim.mo?

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 tried, but our checker complains about await's in prim, so I just moved it here instead. Sneaky.

@@ -593,6 +593,14 @@ let rec check_exp env (exp:Ir.exp) : unit =
error env exp1.at "expected function type, but expression produces type\n %s"
(T.string_of_typ_expand t1)
end
(* TODO: T.unit <: t ? *)
Copy link
Contributor

@ggreif ggreif Jan 30, 2022

Choose a reason for hiding this comment

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

still relevant? for ICCallPrim, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just noticed the missing check

(* TODO: T.unit <: t ? *)
| ICCallRawPrim, [exp1; exp2; exp3; k; r] ->
typ exp1 <: T.principal;
typ exp2 <: T.text;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make this T.blob and normalise Text -> Blob in Motoko?

@rossberg
Copy link
Contributor

There is no assumption that the contents of either blob is Candid, so this could also be used to talk to non-Candid endpoints.

For the record, I'm really concerned about the slippery slope this creates towards leaking and breaking more abstractions.

@skilesare
Copy link

Maybe there is a way to close the abstraction later? This is exactly what we need right now to develop some things on our road map using motoko that have been blocked since launch. Thank you!

@crusso
Copy link
Contributor Author

crusso commented Jan 31, 2022

There is no assumption that the contents of either blob is Candid, so this could also be used to talk to non-Candid endpoints.

For the record, I'm really concerned about the slippery slope this creates towards leaking and breaking more abstractions.

Objection noted.

However, we had promised a solution for ages and never delivered one, so offering this as an experimental feature while we come up with something better seems reasonable to me. Given that you can already achieve this (modulo a change in caller) by bouncing off Rust, I don't really see it compromising more than we have already across the company.

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.

Raw Calls - What is necessary?
5 participants