search

dfinity / cycles-wallet

DFINITY Cycles Wallet
Apache License 2.0
55 stars 30 forks source link

Enhancement: stronger typing of `wallet_send` #50

Closed crusso closed 2 years ago

crusso commented 3 years ago

This is the current Candid for wallet_send

   wallet_send: (record { canister: principal; amount: nat64 }) -> ();

At the moment, it's just taking a principal, p, with no way of saying what interface that principal is expected to support. wallet_rs.wallet_send then blindly calls method p.wallet_receive() assuming it returns some result.

I think the wallet API could (perhaps) use a more strongly typed wallet_send method, something like the higher-order :

   wallet_send: (record { callback: () -> record { accepted : nat64 }; amount: nat64 }) -> ()

(excuse my Candid)

to make the contract clearer and safer. If we want to insist on the wallet_receive name, then the argument could be whole service with a single method instead.

  wallet_send: (record { canister: service { wallet_receive: () -> record { accepted : nat64 } }; amount: nat64 }) -> ()

Discovered debugging https://github.com/dfinity/examples/pull/71 (@lsgunnlsgunn hello_cycles example).

hansl commented 3 years ago

@crusso How do you specify the callback in dfx or frontend or rust? What is a function reference in Candid encoding?

nomeata commented 3 years ago

Candid textual format can express function values via the syntax func "w7x7r-cok77-xa"."method_name"

crusso commented 3 years ago

@lsgunnlsgunn is already tripping over the weak typing. She implemented an actor with a wallet_receive method with the wrong return type, but the rust wallet ignores the result so it actually didn't matter (for now).

https://github.com/dfinity/examples/pull/71#pullrequestreview-614820114

On the other hand, since the candid values for actor and functions are untyped, I guess we only gain a more descriptive wallet interface but no actual additional safety when used from dfx rather than, say, a well-typed Motoko program.

adamspofford-dfinity commented 2 years ago

Resolved in #130 - arbitrary canister functions are no longer called by the wallet_send function, which instead calls the management canister's deposit_cycles function.