Update the token-actor interface #75

alexytsu · 2022-08-18T04:20:03Z

filecoin-project/FIPs#407 should be sync'd with the new interface/types defined here

alexytsu · 2022-08-18T04:55:03Z

testing/fil_token_integration/actors/basic_token_actor/src/lib.rs

+        self.util.transfer(&spender, &spender, &params.to, &params.amount, &params.data)?;
+
+        // FIXME: populate with real values
+        Ok(TransferReturn { from_balance: TokenAmount::zero(), to_balance: TokenAmount::zero() })


might be more efficient if Token::transfer returned the relevant info, else we can make separate calls to Token::balance_of and Token::allowance etc.

Agreed. Simple but maybe error-prone would be to return an anonymous tuple. Another alternative is to return the Return types defined earlier, but that inverts the expected dependency hierarchy. Another option is separate return structs, but they'll be a bit duplicative.

fil_fungible_token/src/receiver/types.rs

anorth

Great improvement, thanks.

fil_fungible_token/src/receiver/types.rs

fil_fungible_token/src/token/types.rs

anorth · 2022-08-18T07:45:09Z

testing/fil_token_integration/actors/basic_token_actor/src/lib.rs

+        self.util.transfer(&spender, &spender, &params.to, &params.amount, &params.data)?;
+
+        // FIXME: populate with real values
+        Ok(TransferReturn { from_balance: TokenAmount::zero(), to_balance: TokenAmount::zero() })


Agreed. Simple but maybe error-prone would be to return an anonymous tuple. Another alternative is to return the Return types defined earlier, but that inverts the expected dependency hierarchy. Another option is separate return structs, but they'll be a bit duplicative.

fil_fungible_token/src/receiver/types.rs

fil_fungible_token/src/token/types.rs

alexytsu · 2022-08-23T01:59:05Z

fil_fungible_token/src/token/mod.rs

+#[derive(Debug)]
+pub enum TransferResult {
+    Transfer(TransferReturn),
+    TransferFrom(TransferFromReturn),
+}
+
+impl TryFrom<TransferResult> for TransferFromReturn {
+    type Error = TokenError;
+
+    fn try_from(value: TransferResult) -> Result<TransferFromReturn> {
+        match value {
+            TransferResult::TransferFrom(ret) => Ok(ret),
+            TransferResult::Transfer(_) => Err(TokenError::TransferReturn { result: value }),
+        }
+    }
+}
+
+impl TryFrom<TransferResult> for TransferReturn {
+    type Error = TokenError;
+
+    fn try_from(value: TransferResult) -> Result<TransferReturn> {
+        match value {
+            TransferResult::TransferFrom(_) => Err(TokenError::TransferReturn { result: value }),
+            TransferResult::Transfer(ret) => Ok(ret),
+        }
+    }
+}
+
+#[derive(Debug)]
+pub enum BurnResult {
+    Burn(BurnReturn),
+    BurnFrom(BurnFromReturn),
+}
+
+impl TryFrom<BurnResult> for BurnReturn {
+    type Error = TokenError;
+
+    fn try_from(value: BurnResult) -> Result<Self> {
+        match value {
+            BurnResult::Burn(ret) => Ok(ret),
+            BurnResult::BurnFrom(_) => Err(TokenError::BurnReturn { result: value }),
+        }
+    }
+}
+
+impl TryFrom<BurnResult> for BurnFromReturn {
+    type Error = TokenError;
+
+    fn try_from(value: BurnResult) -> Result<Self> {
+        match value {
+            BurnResult::BurnFrom(ret) => Ok(ret),
+            BurnResult::Burn(_) => Err(TokenError::BurnReturn { result: value }),
+        }
+    }
+}
+


An alternative to this might be to have separate library level methods for transfer and transfer_from which return different types that match the FRC interface

This enum technique seems like it's trying too hard, and the Err cases are weird. If we're going to have this much code anyway, I think the naive duplication might be better.

fil_fungible_token/src/receiver/types.rs

fil_fungible_token/src/token/mod.rs

anorth · 2022-08-23T02:53:34Z

fil_fungible_token/src/token/mod.rs

+#[derive(Debug)]
+pub enum TransferResult {
+    Transfer(TransferReturn),
+    TransferFrom(TransferFromReturn),
+}
+
+impl TryFrom<TransferResult> for TransferFromReturn {
+    type Error = TokenError;
+
+    fn try_from(value: TransferResult) -> Result<TransferFromReturn> {
+        match value {
+            TransferResult::TransferFrom(ret) => Ok(ret),
+            TransferResult::Transfer(_) => Err(TokenError::TransferReturn { result: value }),
+        }
+    }
+}
+
+impl TryFrom<TransferResult> for TransferReturn {
+    type Error = TokenError;
+
+    fn try_from(value: TransferResult) -> Result<TransferReturn> {
+        match value {
+            TransferResult::TransferFrom(_) => Err(TokenError::TransferReturn { result: value }),
+            TransferResult::Transfer(ret) => Ok(ret),
+        }
+    }
+}
+
+#[derive(Debug)]
+pub enum BurnResult {
+    Burn(BurnReturn),
+    BurnFrom(BurnFromReturn),
+}
+
+impl TryFrom<BurnResult> for BurnReturn {
+    type Error = TokenError;
+
+    fn try_from(value: BurnResult) -> Result<Self> {
+        match value {
+            BurnResult::Burn(ret) => Ok(ret),
+            BurnResult::BurnFrom(_) => Err(TokenError::BurnReturn { result: value }),
+        }
+    }
+}
+
+impl TryFrom<BurnResult> for BurnFromReturn {
+    type Error = TokenError;
+
+    fn try_from(value: BurnResult) -> Result<Self> {
+        match value {
+            BurnResult::BurnFrom(ret) => Ok(ret),
+            BurnResult::Burn(_) => Err(TokenError::BurnReturn { result: value }),
+        }
+    }
+}
+


This enum technique seems like it's trying too hard, and the Err cases are weird. If we're going to have this much code anyway, I think the naive duplication might be better.

fil_fungible_token/src/token/mod.rs

anorth · 2022-08-23T02:59:41Z

fil_fungible_token/src/token/types.rs

@@ -18,172 +18,205 @@ pub enum ActorError<Err> {
 pub type Result<T, E> = std::result::Result<T, ActorError<E>>;


This will be tricky for users to implement, primarily because of the ActorError type defined here. E.g. the builtin actors already return a Result<T, ActorError> but my ActorError is a different type than this. Can this trait be fully generic over the result's error type?

Also, we don't strictly need this trait. It's helpful to ensure full implementation, but I've done fine without it. If we can't get it to be universally helpful to users, we could drop it.

I'll do some thinking and experimenting around this but deferred for a followup

* update the public token-actor interface * update example actor to match new interface * check receiver hooks * cleanup naming * align with FRC-0046 * fix CI build * exclude wasm actors from code coverage * rearrange params * use RawBytes rather than Default * split burn/burn_from transfer/transfer_from

* add integration test contract * write example basic token actor * wip * add build script for example actor * update example actor to latest token api * lock fvm version for example actor test also disables default features because we don't need GPU support (or the dependencies that brings) for running tests * add fil_integration_tests to Makefile * pretty-print the message results in token actor test * use a helper function to simplify messaging in tests * add a barebones actor to implement a receiver hook * extend the mint implementation and mint tokens to the receiver actor * Update the token-actor interface (#75) * update the public token-actor interface * update example actor to match new interface * check receiver hooks * cleanup naming * align with FRC-0046 * fix CI build * exclude wasm actors from code coverage * rearrange params * use RawBytes rather than Default * split burn/burn_from transfer/transfer_from * Split receiver hook calls from minting and transfer - part 1 (#76) * wip: split minting and receiver hook calling, save state at actor level * simplify call_receiver_hook * don't try to reset state on minting failure, let invoke deal with it * set a panic hook to use the vm::abort like built-in actors do this means we'll get back meaningful errors if an unwrap() explodes * token code cleanup * gracefully handle empty return data on minting failure in mint_tokens test * further cleanup * use a more appropriate error code for "unknown method" abort * split receiver hook call out of transfer and transfer_from * remove call_receiver_hook_or_revert * ignore failing mint/transfer tests They're expecting the old behaviour of reverting state (instead of aborting) when a mint or transfer (or receiver hook) fails. Once there's a new method of calling the receiver hook (through a type returned by mint or transfer calls) they can either be adapted to that or rewritten as integration tests * update dispatch crate name * code cleanup * set_root after flushing state in method calls, so the state tree is kept up to date Co-authored-by: Andrew Bright <abright@users.noreply.github.com>

alexytsu force-pushed the token/interface-updates branch 3 times, most recently from 2d170f8 to 712da5f Compare August 18, 2022 04:38

alexytsu added 2 commits August 18, 2022 14:39

update the public token-actor interface

2cb26fe

update example actor to match new interface

e3f0cbf

alexytsu force-pushed the token/interface-updates branch from 712da5f to e3f0cbf Compare August 18, 2022 04:53

alexytsu commented Aug 18, 2022

View reviewed changes

check receiver hooks

e6af2bd

alexytsu commented Aug 18, 2022

View reviewed changes

fil_fungible_token/src/receiver/types.rs Outdated Show resolved Hide resolved

alexytsu requested a review from anorth August 18, 2022 05:15

anorth reviewed Aug 18, 2022

View reviewed changes

cleanup naming

9bc01b1

alexytsu requested a review from abright August 22, 2022 07:27

alexytsu marked this pull request as ready for review August 22, 2022 07:28

alexytsu force-pushed the token/interface-updates branch from b054216 to 9bc01b1 Compare August 22, 2022 07:37

anorth reviewed Aug 22, 2022

View reviewed changes

fil_fungible_token/src/receiver/types.rs Outdated Show resolved Hide resolved

fil_fungible_token/src/token/types.rs Outdated Show resolved Hide resolved

align with FRC-0046

291158b

alexytsu force-pushed the token/interface-updates branch from 9452fe6 to d8a8129 Compare August 23, 2022 01:18

fix CI build

a1e2175

alexytsu force-pushed the token/interface-updates branch from d8a8129 to a1e2175 Compare August 23, 2022 01:22

alexytsu commented Aug 23, 2022

View reviewed changes

exclude wasm actors from code coverage

eaece7e

alexytsu force-pushed the token/interface-updates branch from 72b6095 to eaece7e Compare August 23, 2022 02:00

alexytsu requested a review from anorth August 23, 2022 02:04

anorth approved these changes Aug 23, 2022

View reviewed changes

alexytsu added 3 commits August 23, 2022 13:31

rearrange params

c82c440

use RawBytes rather than Default

2c4577f

split burn/burn_from transfer/transfer_from

762b391

alexytsu mentioned this pull request Aug 23, 2022

Make the FRC interface/trait generic on errors #77

Closed

alexytsu merged commit 4940dfb into token/basic-example Aug 23, 2022

alexytsu deleted the token/interface-updates branch August 23, 2022 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the token-actor interface #75

Update the token-actor interface #75

alexytsu commented Aug 18, 2022

alexytsu Aug 18, 2022

anorth Aug 18, 2022

anorth left a comment

anorth Aug 18, 2022

alexytsu Aug 23, 2022

anorth Aug 23, 2022

anorth Aug 23, 2022

anorth Aug 23, 2022

alexytsu Aug 23, 2022

		@@ -18,172 +18,205 @@ pub enum ActorError<Err> {
		pub type Result<T, E> = std::result::Result<T, ActorError<E>>;

Update the token-actor interface #75

Update the token-actor interface #75

Conversation

alexytsu commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment