FRAME: Conformance tests for fungible API

[gavofyork]

Generic acceptance tests for the fungible::* API, so any implementation can check it does the right thing.

Fungible API

• Regular: Inspect and Mutate traits fungible conformance tests: Inspect and Mutate substrate#13852
• Regular: Unbalanced and Balanced traits Unbalanced and Balanced fungible conformance tests, and fungible fixes #1296
• Hold traits
• Freeze traits

Fungibles API

• Write a new task list here describing TODOs to test fungibles. Should reuse as much code from the fungible api tests as makes sense (see fungible/item_of.rs)

[liamaharon]

I'll pick this one up, planning to start on it next week.

Plan:

1. Review fungible documentation and code, and if required clear up any ambiguity in the specification
2. Create a candidate list of conformance tests and scaffold some code, get it looked over by another member of the FRAME team
3. Implement conformance tests for the balances pallet
4. Abstract balances pallet conformance tests into a generalised macro which can be used something like conformance_tests!(Implementor)

[liamaharon]

My current thinking:

• We should segment tests by trait (e.g. one for Mutate, one for Balanced, one for MutateHold, etc.), so they can be selectively applied only for traits implemented by the pallet
• Each trait test should begin by thoroughly unit testing each trait method, and finish with any integration tests deemed necessary to achieve full coverage
• The tests should live somewhere in frame/support/src/traits/tokens/fungible, perhaps

frame/support/src/traits/tokens/fungible/
├── freeze.rs
├── hold.rs
├── item_of.rs
├── mod.rs
├── regular.rs
└── conformance_tests/
    ├── balanced_hold.rs
    ├── balanced.rs
    ├── handle_imbalance_drop.rs
    ├── inspect_freeze.rs
    ├── inspect_hold.rs
    ├── inspect.rs
    ├── mod.rs
    ├── mutate_freeze.rs
    ├── mutate_hold.rs
    └── mutate.rs

• The tests can be applied either
  • one macro per-trait, e.g. fungible_inspect_conformance_tests!(PalletName) or
  • using a single meta-macro that inspects the pallet to determine which trait-specific macros are appropriate and auto-implements them, e.g. fungible_conformance_tests!(PalletName)

In terms of implementation steps, I'm thinking to methodically walk through each trait and test each method, initially coupling things to balances. Once the test surface is solid, begin the process of de-coupling & converting the tests into macros. edit: implement Inspect and Mutate tests directly on the Balances pallet, check with the team my testing approach is sensible, then if OK abstract the tests into macros and proceed with writing tests for the remaining traits.

Open Questions

1. Does the pattern of segmenting tests (and macros) by fungible trait make sense?

   • As I write this I'm becoming unsure about this approach, because some traits seem to depend on another. Example: how is it possible to test total_balance from the Inspect trait if the pallet doesn't also implement Mutate allowing us to use set_balance in the test?
   • Edit: I just spoke about this with Keith. We agreed that extreme edge cases where a pallet does something like implementing Inspect without Mutate are not worth supporting (at least initially). In most cases we are covered by supertraits anyway, e.g. if a pallet implements Mutate we know that Inspect + Unbalanced are also implemented.

2. Do we need to add conformance tests for Unbalanced traits? Based on the documentation I would think no, because it is not advised for pallets to use them (they should use Balanced instead). However I noticed that the balances pallet implements Unbalanced for some operations, so am unsure.

3. How could the de-coupling of conformance tests & abstraction into macros be done? (this doesn't need to be addressed immediately, but when I do get to it I'd be grateful for some help, probably from @sam0x17 and/or @KiChjang).

[kianenigma]

TODO: What does it mean for some issuance to be 'controlled by the system'?

eg. locked and controlled by the crowdloan system.

The tests should live somewhere in frame/support/src/traits/tokens/fungible, perhaps

The provided branch puts them under frame/balances though.

[liamaharon]

I've made progress on Inspect and Mutate tests, and refactored them into macros exported from frame_support on this very WIP, PoC branch.

I have a bunch of questions such as (non-exhaustive)

• Where do we draw the line on assuming pallet implementation details? e.g. existential deposit is only mentioned in passing in some comments in the fungibles crate, but it (or something like it) is required if we'd like to test parameters on some functions such as preservation on Mutate::decrease_balance
• Best practices on Rust testing patterns, e.g. testing that done_burn_from is called after burn_from
• Naming (modules, files, macros, etc)
• Usage of use in macros
• Is my general approach here making sense, is there anything I am missing?

I think they'd be best covered over a call. @kianenigma if you have time this week perhaps we could schedule 30 minutes in?

Once these questions have been cleared up and the PoC smoothed over, I think I'm ready to create a draft PR from my branch tracking TODOs and progress.

[xlc]

I will suggest don't try to implement everything in one go. Focus on a few traits and push it. Then we can also try to integrate them in ORML to check the implementation of orml-tokens. We will likely have feedbacks based on real usages.

[liamaharon]

Spoke with Kian about this. Going to:

• Refactor the macros into simple functions which take a struct implementing the trait/s they will test
• Remove assumptions about an 'existential deposit', just the existing minimum_balance (I had missed that they were the same)
• Begin progressively shipping trait tests, as also suggested by @xlc

[juangirini]

@liamaharon I see a close PR, does that closes this one?

[liamaharon]

@juangirini there're still some traits that need coverage, I'll update the issue description to track them