ERC-721 example: fix panic re: approve_for with nonexistent token (#2092)

* ERC-721 example: fix panic re: `approve_for` with nonexistent token

* Update changelog

* Refactor token_transfer_from

Author: zgrannan

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Fix the `StorageVec` type by excluding the `len_cached` field from its type info - [#2052](https://github.com/paritytech/ink/pull/2052)
+- Fix panic in `approve_for` in the ERC-721 example - [#2092](https://github.com/paritytech/ink/pull/2092)
 - ERC-721: `transfer_token_from` now ensures the token owner is correct - [#2093](https://github.com/paritytech/ink/pull/2093)
 
 ## Version 5.0.0-rc

integration-tests/erc721/lib.rs

@@ -217,7 +217,7 @@ mod erc721 {
 
             let owner = token_owner.get(id).ok_or(Error::TokenNotFound)?;
             if owner != caller {
-                return Err(Error::NotOwner)
+                return Err(Error::NotOwner);
             };
 
             let count = owned_tokens_count
@@ -244,14 +244,12 @@ mod erc721 {
             id: TokenId,
         ) -> Result<(), Error> {
             let caller = self.env().caller();
-            if !self.exists(id) {
-                return Err(Error::TokenNotFound)
+            let owner = self.owner_of(id).ok_or(Error::TokenNotFound)?;
+            if !self.approved_or_owner(caller, id, owner) {
+                return Err(Error::NotApproved);
             };
-            if !self.approved_or_owner(Some(caller), id) {
-                return Err(Error::NotApproved)
-            };
-            if self.token_owner.get(id) != Some(*from) {
-                return Err(Error::NotOwner)
+            if owner != *from {
+                return Err(Error::NotOwner);
             };
             self.clear_approval(id);
             self.remove_token_from(from, id)?;
@@ -277,7 +275,7 @@ mod erc721 {
             } = self;
 
             if !token_owner.contains(id) {
-                return Err(Error::TokenNotFound)
+                return Err(Error::TokenNotFound);
             }
 
             let count = owned_tokens_count
@@ -299,11 +297,11 @@ mod erc721 {
             } = self;
 
             if token_owner.contains(id) {
-                return Err(Error::TokenExists)
+                return Err(Error::TokenExists);
             }
 
             if *to == AccountId::from([0x0; 32]) {
-                return Err(Error::NotAllowed)
+                return Err(Error::NotAllowed);
             };
 
             let count = owned_tokens_count
@@ -325,7 +323,7 @@ mod erc721 {
         ) -> Result<(), Error> {
             let caller = self.env().caller();
             if to == caller {
-                return Err(Error::NotAllowed)
+                return Err(Error::NotAllowed);
             }
             self.env().emit_event(ApprovalForAll {
                 owner: caller,
@@ -346,19 +344,17 @@ mod erc721 {
         /// the message's sender.
         fn approve_for(&mut self, to: &AccountId, id: TokenId) -> Result<(), Error> {
             let caller = self.env().caller();
-            let owner = self.owner_of(id);
-            if !(owner == Some(caller)
-                || self.approved_for_all(owner.expect("Error with AccountId"), caller))
-            {
-                return Err(Error::NotAllowed)
+            let owner = self.owner_of(id).ok_or(Error::TokenNotFound)?;
+            if !(owner == caller || self.approved_for_all(owner, caller)) {
+                return Err(Error::NotAllowed);
             };
 
             if *to == AccountId::from([0x0; 32]) {
-                return Err(Error::NotAllowed)
+                return Err(Error::NotAllowed);
             };
 
             if self.token_approvals.contains(id) {
-                return Err(Error::CannotInsert)
+                return Err(Error::CannotInsert);
             } else {
                 self.token_approvals.insert(id, to);
             }
@@ -389,20 +385,16 @@ mod erc721 {
 
         /// Returns true if the `AccountId` `from` is the owner of token `id`
         /// or it has been approved on behalf of the token `id` owner.
-        fn approved_or_owner(&self, from: Option<AccountId>, id: TokenId) -> bool {
-            let owner = self.owner_of(id);
-            from != Some(AccountId::from([0x0; 32]))
+        fn approved_or_owner(
+            &self,
+            from: AccountId,
+            id: TokenId,
+            owner: AccountId,
+        ) -> bool {
+            from != AccountId::from([0x0; 32])
                 && (from == owner
-                    || from == self.token_approvals.get(id)
-                    || self.approved_for_all(
-                        owner.expect("Error with AccountId"),
-                        from.expect("Error with AccountId"),
-                    ))
-        }
-
-        /// Returns true if token `id` exists or false if it does not.
-        fn exists(&self, id: TokenId) -> bool {
-            self.token_owner.contains(id)
+                    || self.token_approvals.get(id) == Some(from)
+                    || self.approved_for_all(owner, from))
         }
     }
 
@@ -563,6 +555,16 @@ mod erc721 {
             assert!(!erc721.is_approved_for_all(accounts.alice, accounts.bob));
         }
 
+        #[ink::test]
+        fn approve_nonexistent_token_should_fail() {
+            let accounts =
+                ink::env::test::default_accounts::<ink::env::DefaultEnvironment>();
+            // Create a new contract instance.
+            let mut erc721 = Erc721::new();
+            // Approve transfer of nonexistent token id 1
+            assert_eq!(erc721.approve(accounts.bob, 1), Err(Error::TokenNotFound));
+        }
+
         #[ink::test]
         fn not_approved_transfer_should_fail() {
             let accounts =