shawntabrizi / substrate-collectables-workshop

A guided tutorial for building an NFT marketplace with the Polkadot SDK
https://www.shawntabrizi.com/substrate-collectables-workshop/
MIT License
236 stars 101 forks source link

Should breeding a kitty use checked_add for incremented gen value? #122

Closed todofixthis closed 3 months ago

todofixthis commented 5 years ago

In https://substrate.dev/substrate-collectables-workshop/#/3/breeding-a-kitty the solution uses the following code to set the new kitty's gen value:

            let new_kitty = Kitty {
                ...
                gen: cmp::max(kitty_1.gen, kitty_2.gen) + 1,
            };

Should this logic be using checked_add() instead (concept introduced in chapter 2)? E.g.:

            let kitty_1 = Self::kitty(kitty_id_1);
            let kitty_2 = Self::kitty(kitty_id_2);

            let new_gen = cmp::max(kitty_1.gen, kitty_2.gen).checked_add(1)
                .ok_or("Overflow when incrementing offspring kitty's generation number.")?;

            ...

            let new_kitty = Kitty {
                ...
                gen: new_gen,
            };

I'm new to Rust and Substrate, so I don't know if this is a special case where checked_add() is not necessary — then this would be an opportunity to reinforce the principle (or explain the special case).

shawntabrizi commented 5 years ago

It should be used here too for safety. However, in this case gen can never be greater than the number of kitties in the system. And both gen and total_kitties_count use the same u64 to store the value.

If we introduced kitty deletion, then we may have a problem... either way, doing saturated_add is probably what we want here. Basically it will get to a max value, and not increase anymore.