pba-content icon indicating copy to clipboard operation
pba-content copied to clipboard

[PBA2] FRAME Assignment common issues

Open kianenigma opened this issue 2 years ago • 4 comments
trafficstars

The exam showed that students, again, despite passing the rust exam, make a lot of mistake in using Options and Results. This becomes crucial next to "no panic in Runtime" fact.

This example from shawn should be hidden in the exam:

Here is a small primer:

#[derive(Default)]
struct StorageValue {
    value: Option<u32>,
}
impl StorageValue {
    pub fn get(&self) -> Option<u32> {
        self.value
    }
    
    pub fn set(mut self, v: Option<u32>) {
        self.value = v;
    }
    
    pub fn put(mut self, v: u32) {
        self.value = Some(v);
    }
}

enum PalletError {
    NoValueSet,
}

fn my_function() -> Result<(), PalletError> {
    let MyStorage = StorageValue::default();
    
    // Read some storage, if NONE return an error
    let value = MyStorage.get().ok_or(PalletError::NoValueSet)?;
    
    // Anti-pattern. Don't do this!
    let maybe_value = MyStorage.get();
    if maybe_value.is_none() {
        return Err(PalletError::NoValueSet)
    }
    let value = maybe_value.unwrap();
    // This is at least a little better, but still an anti-pattern
    let value = maybe_value.expect("cannot panic because of `none` check right above. q.e.d");
    
    // Do some logic only if a storage value exists, but don't return an error
    // which would halt the whole extrinsic.
    if let Some(value) = MyStorage.get() {
        // do things with value
        // If there is no value, then this code will never execute.
    }
    
    // This is okay... but probably means you should be using `ValueQuery` configuration.
    // Can't really make a simple example here of `ValueQuery`. Check the presentations.
    // So probably again an anti-pattern.
    let value = MyStorage.get().unwrap_or_default();
    
    Ok(())
}

kianenigma avatar Feb 01 '23 18:02 kianenigma

Another example that I see really really ugly code written using try_mutate and try_mutate_exists, where it is not justified. I am worried that students:

  1. don't know the full list of functions and APIs of storage in mind when writing this
  2. think that just because mutate_x has a closure, it more advance.

We should make it clear that these functions were specifically useful for the days of pre-transactional-by-default.

Perhaps, we should even discourage using them unless if absolutely needed.

kianenigma avatar Feb 01 '23 19:02 kianenigma

So we should have a whole module in the rust entrance exam covering options and results and crating test for those ( we test the tests they write somehow?)

nuke-web3 avatar Feb 03 '23 11:02 nuke-web3

Ideas for the Uni V2 dex that we could encourage students to explore:

  • pair of assets, what do you do with balances / native currency?
    • feeless?
    • remove balances and only assets?
    • replace native currency for fees with assets?
  • liquidity pool is an asset...
    • deterministic pool -> asset ID? or to assets managed by some key-less account?
      • if assetID... then people could squat on IDs and block liquidity pool rewards token! Wat do? (instantiable pallets, one for user tokens, one for liquidity pool rewards... how stop userspace? call filters?)

Lots and lots of fun ways to push people to dive deeper with this project than a minimal impl.

nuke-web3 avatar Feb 03 '23 15:02 nuke-web3

replace native currency for fees with assets

I did this in a seminar a few years back. It was very enlightening to do so. Could be a good challenge for students.

JoshOrndorff avatar Feb 03 '23 22:02 JoshOrndorff