fix(erc721): properly handle receiver errors (#225)

Resolves #224 

The `FIXME`s added in the PR are tracked
[here](#228).

- [x] Tests

contracts/src/token/erc721/mod.rs

@@ -3,7 +3,11 @@ use alloc::vec;
 
 use alloy_primitives::{fixed_bytes, uint, Address, FixedBytes, U128, U256};
 use stylus_sdk::{
-    abi::Bytes, alloy_sol_types::sol, call::Call, evm, msg, prelude::*,
+    abi::Bytes,
+    alloy_sol_types::sol,
+    call::{self, Call},
+    evm, msg,
+    prelude::*,
 };
 
 use crate::utils::math::storage::{AddAssignUnchecked, SubAssignUnchecked};
@@ -133,6 +137,9 @@ pub enum Error {
     InvalidSender(ERC721InvalidSender),
     /// Indicates a failure with the token `receiver`. Used in transfers.
     InvalidReceiver(ERC721InvalidReceiver),
+    /// Indicates a failure with the token `receiver`, with the reason
+    /// specified by it.
+    InvalidReceiverWithReason(call::Error),
     /// Indicates a failure with the `operator`’s approval. Used in transfers.
     InsufficientApproval(ERC721InsufficientApproval),
     /// Indicates a failure with the `approver` of a token to be approved. Used
@@ -1051,11 +1058,10 @@ impl Erc721 {
     /// `operator` is generally the address that initiated the token transfer
     /// (i.e. `msg::sender()`).
     ///
-    /// The acceptance call is not executed and treated as a no-op
-    /// if the target address doesn't contain code (i.e. an EOA).
-    /// Otherwise, the recipient must implement
-    /// [`IERC721Receiver::on_erc_721_received`] and return the
-    /// acceptance magic value to accept the transfer.
+    /// The acceptance call is not executed and treated as a no-op if the
+    /// target address doesn't contain code (i.e. an EOA). Otherwise, the
+    /// recipient must implement [`IERC721Receiver::on_erc_721_received`] and
+    /// return the acceptance magic value to accept the transfer.
     ///
     /// # Arguments
     ///
@@ -1080,29 +1086,37 @@ impl Erc721 {
         token_id: U256,
         data: &Bytes,
     ) -> Result<(), Error> {
-        const IERC721RECEIVER_INTERFACE_ID: FixedBytes<4> =
-            fixed_bytes!("150b7a02");
-
-        // FIXME: Cleanup this code once it's covered in the test suite.
-        if to.has_code() {
-            let call = Call::new_in(self);
-            return match IERC721Receiver::new(to).on_erc_721_received(
-                call,
-                operator,
-                from,
-                token_id,
-                data.to_vec(),
-            ) {
-                Ok(result) => {
-                    if result == IERC721RECEIVER_INTERFACE_ID {
-                        Ok(())
-                    } else {
-                        Err(ERC721InvalidReceiver { receiver: to }.into())
+        const RECEIVER_FN_SELECTOR: FixedBytes<4> = fixed_bytes!("150b7a02");
+
+        if !to.has_code() {
+            return Ok(());
+        }
+
+        let receiver = IERC721Receiver::new(to);
+        let call = Call::new_in(self);
+        let data = data.to_vec();
+        let result =
+            receiver.on_erc_721_received(call, operator, from, token_id, data);
+
+        let id = match result {
+            Ok(id) => id,
+            Err(e) => {
+                if let call::Error::Revert(ref reason) = e {
+                    if reason.len() > 0 {
+                        // Non-IERC721Receiver implementer.
+                        return Err(Error::InvalidReceiverWithReason(e));
                     }
                 }
-                Err(_) => Err(ERC721InvalidReceiver { receiver: to }.into()),
-            };
+
+                return Err(ERC721InvalidReceiver { receiver: to }.into());
+            }
+        };
+
+        // Token rejected.
+        if id != RECEIVER_FN_SELECTOR {
+            return Err(ERC721InvalidReceiver { receiver: to }.into());
         }
+
         Ok(())
     }
 }

examples/erc721/tests/erc721.rs

@@ -737,9 +737,6 @@ async fn error_when_safe_transfer_nonexistent_token(
     Ok(())
 }
 
-// TODO: Test reverts & panics on ERC721ReceiverMock for
-// `Erc721::safeTransferFrom_0`.
-
 #[e2e::test]
 async fn safe_transfers_from_with_data(
     alice: Account,
@@ -1092,8 +1089,102 @@ async fn error_when_safe_transfer_with_data_nonexistent_token(
     Ok(())
 }
 
-// TODO: Test reverts & panics on ERC721ReceiverMock for
-// `Erc721::safeTransferFrom_1`.
+// FIXME: Update our `reverted_with` implementation such that we can also check
+// when the error is a `stylus_sdk::call::Error`.
+#[e2e::test]
+#[ignore]
+async fn errors_when_receiver_reverts_with_reason(
+    alice: Account,
+) -> eyre::Result<()> {
+    let contract_addr = deploy(alice.url(), &alice.pk()).await?;
+    let contract = Erc721::new(contract_addr, &alice.wallet);
+
+    let receiver_address = receiver::deploy(
+        &alice.wallet,
+        ERC721ReceiverMock::RevertType::RevertWithMessage,
+    )
+    .await?;
+
+    let alice_addr = alice.address();
+    let token_id = random_token_id();
+
+    let _ = watch!(contract.mint(alice_addr, token_id))?;
+
+    let _err = send!(contract.safeTransferFrom_0(
+        alice_addr,
+        receiver_address,
+        token_id
+    ))
+    .expect_err("should not transfer when receiver errors with reason");
+
+    // assert!(err.reverted_with(stylus_sdk::call::Error::Revert(
+    //     b"ERC721ReceiverMock: reverting".to_vec()
+    // )));
+    Ok(())
+}
+
+#[e2e::test]
+async fn errors_when_receiver_reverts_without_reason(
+    alice: Account,
+) -> eyre::Result<()> {
+    let contract_addr = deploy(alice.url(), &alice.pk()).await?;
+    let contract = Erc721::new(contract_addr, &alice.wallet);
+
+    let receiver_address = receiver::deploy(
+        &alice.wallet,
+        ERC721ReceiverMock::RevertType::RevertWithoutMessage,
+    )
+    .await?;
+
+    let alice_addr = alice.address();
+    let token_id = random_token_id();
+
+    let _ = watch!(contract.mint(alice_addr, token_id))?;
+
+    let err = send!(contract.safeTransferFrom_0(
+        alice_addr,
+        receiver_address,
+        token_id
+    ))
+    .expect_err("should not transfer when receiver reverts");
+
+    assert!(err.reverted_with(Erc721::ERC721InvalidReceiver {
+        receiver: receiver_address
+    }));
+
+    Ok(())
+}
+
+// FIXME: Update our `reverted_with` implementation such that we can also check
+// when the error is a `stylus_sdk::call::Error`.
+#[e2e::test]
+#[ignore]
+async fn errors_when_receiver_panics(alice: Account) -> eyre::Result<()> {
+    let contract_addr = deploy(alice.url(), &alice.pk()).await?;
+    let contract = Erc721::new(contract_addr, &alice.wallet);
+
+    let receiver_address =
+        receiver::deploy(&alice.wallet, ERC721ReceiverMock::RevertType::Panic)
+            .await?;
+
+    let alice_addr = alice.address();
+    let token_id = random_token_id();
+
+    let _ = watch!(contract.mint(alice_addr, token_id))?;
+
+    let err = send!(contract.safeTransferFrom_0(
+        alice_addr,
+        receiver_address,
+        token_id
+    ))
+    .expect_err("should not transfer when receiver panics");
+
+    assert!(err.reverted_with(Erc721::ERC721InvalidReceiver {
+        receiver: receiver_address
+    }));
+
+    Ok(())
+}
 
 #[e2e::test]
 async fn approves(alice: Account, bob: Account) -> eyre::Result<()> {