A new tutorial section added: Auction a Kitty

[arikan]

I've created a tutorial section for auctioning kitties. This could be a useful addition to the tutorial for learning about the concept of time on chain, how to use the substrate specific BlockNumber type, and on_finalise() function for things that needs to be done at the end of a block.

[shawntabrizi]

These are some comments from a first pass, not looking extremely closely, compiling, or testing the code.

Seems good overall, but there is actually a lot more needed to include such a function in a working runtime. This is why an auction system itself is quite a bit more complicated. It might even require its own "section" with multiple chapters.

Are you basing this logic off an existing ethereum smart contract or similar? If not, that might be a good way to get some battle tested logic integrated.

[shawntabrizi]

Do we need kitty_owner? It doesn't take up so much storage, but does not seem necessary since with kitty_id we can always look it up instantly

[arikan]

Right, a more dry storage would be preferable. However, on_finalise does not like options. So, if I use .ok_or()? I get the following error:

let owner = Self::owner_of(auction.kitty_id).ok_or("No owner for this cat")?;

cannot use the `?` operator in a function that returns `()`

If I get the owner without ok_or(), and try to use it in transfer functions, it throws the following:

let owner = Self::owner_of(auction.kitty_id);
let _ = Self::_transfer_from(owner.clone(), auction.high_bidder.clone(), auction.kitty_id);

expected type `<T as system::Trait>::AccountId` found type `std::option::Option<<T as system::Trait>::AccountId>`

[shawntabrizi]

ok_or just transforms an Option from Some(value) to Ok(value) or None to Err(error):
https://doc.rust-lang.org/std/option/enum.Option.html#method.ok_or

When ok_or returns an error, the ? operator handles that by returning the error: https://rust-lang-nursery.github.io/edition-guide/rust-2018/error-handling-and-panics/the-question-mark-operator-for-easier-error-handling.html

However this on_finalise function does not return a value, so you need to instead step back, and handle these two scenarios yourself.

You can do a match statement or you can do something like:

fn function_returns_result() -> Result<(), &'static str> {
    let a: u32 = 5;
    let _b = a.checked_add(20).ok_or("Overflow Error");
    
    Ok(())
}

fn main() {
    if function_returns_result().is_ok() {
        // do your logic
    }
}

Does this help/make sense?

[arikan]

I looked at the explanations of ?, ok_or, match, but not sure how I can use this example. I am new to Rust.

Right, because on_finalise does not return a value, as far as I understand, all I need to do is while doing the auctions loop, check if an owner exists for the kitty_id, if not, skip, if exists, then use it. Something like this does not work for example:

// rust does not like this result statement
fn _ensure_owner<AccountId>(kitty_id: T::Hash) -> Result<AccountId, &'static str> {
        let _owner = Self::owner_of(kitty_id).ok_or("No owner for this cat")?;

        Ok(())
}

// Even if it did work, where do I get the owner?
if Self::_ensure_owner(auction.kitty_id).is_ok() {

}

[shawntabrizi]

I have not actually ran this code, but something like this I think would work:

fn on_finalise() {
    let auctions = Self::auctions_expire_at(<system::Module<T>>::block_number());
    
    for auction in &auctions {
        let owner = Self::owner_of(kitty_id);
        
        let owner = match owner {
            Ok(v) => v,
            Err(e) => continue,
        };
        
        let owned_kitty_count_from = Self::owned_kitty_count(&owner);
        let owned_kitty_count_to = Self::owned_kitty_count(&auction.high_bidder);
        
        if  owned_kitty_count_to.checked_add(1).is_ok() &&
            owned_kitty_count_from.checked_sub(1).is_ok()
        {
            if <balances::Module<T>>::make_transfer(&auction.high_bidder, &owner, auction.high_bid).is_ok() {
                // Guaranteed to work since we did the appropriate checks above
                Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
            } 
        }
    }
    
    for auction in &auctions {
        <KittyAuction<T>>::remove(auction.kitty_id);
        <Auctions<T>>::remove(<system::Module<T>>::block_number());
    }
}

You may need to finesse it, but the ideas are there.

[arikan]

Ok I handled the _transfer_from result with a match:

if <balances::Module<T>>::make_transfer(&auction.high_bidder, &auction.kitty_owner, auction.high_bid).is_ok() {
    let res = Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
    match res {
        Ok(v) => {
            Self::deposit_event(RawEvent::AuctionFinalized(auction.kitty_id, auction.high_bid, auction.expiry));
            v
        },
        Err(_) => continue,
    };
}

[shawntabrizi]

@arikan we do a checked_add but we never actually write to storage, so it will never "double increment".

We have to do it like this because we have two functions which both do checks and write to storage. In serial it would look like this:

1. Function 1 Check
2. Function 1 Write
3. Function 2 Check
4. Function 2 Write

If "Function 2 Check" fails, then we have already written with "Function 1 Write".

Instead we want:

• Function 1 Check
• Function 2 Check
• Function 1 Write
• Function 2 Write

You could be smart and break up your functions to be two halves, so you have:

• _transfer_from_check() and _transfer_from_write

For simplicity here, I am just checking the same stuff as "Function 2 Check" before I call the functions in serial with "Function 1" first, ensuring that "Function 2 Check" will succeed:

• Function 2 Check (Custom code)
• Function 1 Check
• Function 1 Write
• Function 2 Check (We already know it will succeed from above)
• Function 2 Write

This is why I wrote it the way I did.

[shawntabrizi]

I dont think you need to return v at the end? but that looks like what you should be doing

[arikan]

Hi @shawntabrizi, thank you again for the explanation. I integrated these checks and added others (e.g., unreserve check for the auction). However, I wasn't able to realize the transfers for kitty and currency under these checks. I tried three different ways of checking transfers, but none of them realized the transfers. Also, the buy_kitty() method in the tutorial does not use checks for the two interdependent transfers, this should be updated right? I'd appretiate any help here. Thank you.

Checking transfers using match:

fn on_finalise() {
    let auctions = Self::auctions_expire_at(<system::Module<T>>::block_number());
    for auction in &auctions {
        let owned_kitty_count_from = Self::owned_kitty_count(&auction.kitty_owner);
        let owned_kitty_count_to = Self::owned_kitty_count(&auction.high_bidder);
        if owned_kitty_count_to.checked_add(1).is_some() &&
            owned_kitty_count_from.checked_sub(1).is_some() &&
            auction.kitty_owner != auction.high_bidder
        {
            let ureserved = <balances::Module<T>>::unreserve(&auction.high_bidder, auction.high_bid);
            if ureserved.is_some() {
                // This event is deposited
                Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid)); 

                // But these transfers are not realized
                if <balances::Module<T>>::make_transfer(&auction.high_bidder, &auction.kitty_owner, auction.high_bid).is_ok() {
                    let res = Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
                    match res {
                        Ok(v) => {
                            Self::deposit_event(RawEvent::AuctionFinalized(auction.kitty_id, auction.high_bid, auction.expiry));
                            v
                        },
                        Err(_) => continue,
                    };
                }
            }
        }
    }
}

Checking transfers using is_ok():

fn on_finalise() {
    let auctions = Self::auctions_expire_at(<system::Module<T>>::block_number());
    for auction in &auctions {
        let owned_kitty_count_from = Self::owned_kitty_count(&auction.kitty_owner);
        let owned_kitty_count_to = Self::owned_kitty_count(&auction.high_bidder);
        if owned_kitty_count_to.checked_add(1).is_some() &&
            owned_kitty_count_from.checked_sub(1).is_some() &&
            auction.kitty_owner != auction.high_bidder
        {
            let ureserved = <balances::Module<T>>::unreserve(&auction.high_bidder, auction.high_bid);
            if ureserved.is_some() {
                // This event is deposited
                Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid)); 

                // But these transfers are not realized
                let currency_transfer = <balances::Module<T>>::make_transfer(&auction.high_bidder, &auction.kitty_owner, auction.high_bid);
                if currency_transfer.is_ok() {
                    let kitty_transfer = Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
                    if kitty_transfer.is_ok() {
                        Self::deposit_event(RawEvent::AuctionFinalized(auction.kitty_id, auction.high_bid, auction.expiry));
                    }
                }
            }
        }
    }
}

Not checking transfers at all:

fn on_finalise() {
    let auctions = Self::auctions_expire_at(<system::Module<T>>::block_number());
    for auction in &auctions {
        let owned_kitty_count_from = Self::owned_kitty_count(&auction.kitty_owner);
        let owned_kitty_count_to = Self::owned_kitty_count(&auction.high_bidder);
        if owned_kitty_count_to.checked_add(1).is_some() &&
            owned_kitty_count_from.checked_sub(1).is_some() &&
            auction.kitty_owner != auction.high_bidder
        {
            let ureserved = <balances::Module<T>>::unreserve(&auction.high_bidder, auction.high_bid);
            if ureserved.is_some() {
                // This event is deposited
                Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid)); 

                // But these transfers are not realized
                let _ = <balances::Module<T>>::make_transfer(&auction.high_bidder, &auction.kitty_owner, auction.high_bid);
                let _ = Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
                Self::deposit_event(RawEvent::AuctionFinalized(auction.kitty_id, auction.high_bid, auction.expiry));
            }
        }
    }
}

[arikan]

Ok, it finally works. Notice the unreserve check is not done (see the comments). I'd appretiate a suggestion for this check.

fn on_finalise() {
    let auctions = Self::auctions_expire_at(<system::Module<T>>::block_number());
    for auction in &auctions {
        let owned_kitty_count_from = Self::owned_kitty_count(&auction.kitty_owner);
        let owned_kitty_count_to = Self::owned_kitty_count(&auction.high_bidder);
        if owned_kitty_count_to.checked_add(1).is_some() &&
            owned_kitty_count_from.checked_sub(1).is_some() &&
            auction.kitty_owner != auction.high_bidder
        {
            // _transfer_from works if kitty has no auction, so we clear KittyAuction first
            <KittyAuction<T>>::remove(auction.kitty_id);

            // Check required for unreserve, but the methods below do not work
            let _ = <balances::Module<T>>::unreserve(&auction.high_bidder, auction.high_bid);
            Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid));

            // Does not get into this boolean condition somehow
            // if _unreserved.is_some() {
            //     Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid));
            // }

            // Does not compile: expected enum `core::option::Option`, found ()
            // match _unreserved {
            //     None => None,
            //     Some(i) => {
            //         // Self::deposit_event(RawEvent::Unreserved(auction.high_bidder.clone(), auction.high_bid));
            //         Some(i);
            //     },
            // }

            let _currency_transfer = <balances::Module<T>>::make_transfer(&auction.high_bidder, &auction.kitty_owner, auction.high_bid);
            match _currency_transfer {
                Err(_e) => continue,
                Ok(_v) => {
                    let _kitty_transfer = Self::_transfer_from(auction.kitty_owner.clone(), auction.high_bidder.clone(), auction.kitty_id);
                    match _kitty_transfer {
                        Err(_e) => continue,
                        Ok(_v) => {
                            Self::deposit_event(RawEvent::AuctionFinalized(auction.kitty_id, auction.high_bid, auction.expiry));
                        },
                    }
                },
            }
        }
    }

    for auction in &auctions {
        <Auctions<T>>::remove(<system::Module<T>>::block_number());

        let bid_accounts = Self::bid_accounts(auction.kitty_id);

        for account in bid_accounts {
            let bid_balance = Self::bid_of((auction.kitty_id, account.clone()));
            // Checks have to be done here, same problem like above
            let _ = <balances::Module<T>>::unreserve(&account, bid_balance);
            Self::deposit_event(RawEvent::Unreserved(account.clone(), bid_balance));
            <Bids<T>>::remove((auction.kitty_id, account));
        }
        <BidAccounts<T>>::remove(auction.kitty_id);
    }
}

[arikan]

Hi @shawntabrizi, thank you again for all your comments.

I updated the code and created a new section with multiple chapters, which is now easier to digest.

I tested the code using polkadot.js.org and a custom oo7 UI.

Let me know if it works in your local and if you have further suggestions and comments.

[shawntabrizi]

Combine with the line above:

use support::traits::{Currency, ReservableCurrency};

[shawntabrizi]

The content of this chapter still need to be revamped to be a bit more detailed and educational. Will track in another issue.