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

Universal token receiver #93

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Universal token receiver #93

merged 3 commits into from
Sep 6, 2022

Conversation

abright
Copy link
Contributor

@abright abright commented Sep 2, 2022

Implements the universal receiver hook as detailed in FRC-0046

Also makes a change to our FvmMessenger which fixes an issue I had with actor-to-actor messaging. The receiver actor example can now deserialise and inspect details of incoming transfers

Params already come in as `RawBytes` so we don't need to do anything special to them. Fixes deserialisation panics in the example receiver actor
@coveralls
Copy link

coveralls commented Sep 2, 2022

Coverage Status

Coverage increased (+0.3%) to 75.557% when pulling 6fbbf68 on token/universal-receiver into a5a9527 on main.

Copy link
Member

@alexytsu alexytsu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -71,8 +71,7 @@ impl Messaging for FvmMessenger {
params: &RawBytes,
value: &TokenAmount,
) -> Result<Receipt> {
let params = RawBytes::new(fvm_ipld_encoding::to_vec(&params)?);
Ok(send::send(to, method, params, value.clone())?)
Ok(send::send(to, method, params.clone(), value.clone())?)
Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks

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 want to have a further look at this later. I think every time we call this, we're creating a RawBytes at the call site to serialise some params struct. Might be better to just move ownership (of the RawBytes) in that case to avoid the clone and save a little gas.

@@ -12,11 +13,14 @@ pub trait FRC46TokenReceiver {
/// the receiving actor can immediately utilise the received funds. If the receiver wishes to
/// reject the incoming transfer, this function should abort which will cause the token actor
/// to rollback the transaction.
fn tokens_received(params: TokensReceivedParams);
fn receive(token_type: TokenType, params: FRC46TokenReceived);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps generalise this to be a UniversalReceiver trait, and with the params argument here being raw bytes. We're going to want essentially the same thing for NFT (so perhaps this will move out of this crate to a new fil_universal_receiver crate too).

But you could convince me that being generic for all receivers is out of scope here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moving things around can wait for a follow-up of course

Copy link
Contributor Author

@abright abright Sep 6, 2022

Choose a reason for hiding this comment

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

Updated to this:

fn receive(params: UniversalReceiverParams);

I've also added more comments on it and the UniversalReceiverParams which hopefully explain things clearly.

Moving things around can wait for a follow-up of course

Added issue #94 for this

fil_fungible_token/src/receiver/types.rs Outdated Show resolved Hide resolved
fil_fungible_token/src/receiver/types.rs Outdated Show resolved Hide resolved
fil_fungible_token/src/token/mod.rs Show resolved Hide resolved
fil_fungible_token/src/runtime/messaging.rs Outdated Show resolved Hide resolved
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