✅ Comprehensive community round tests (518)#229
Conversation
…-tests # Conflicts: # pallets/funding/src/migration.rs
Test a bid split going over the limits fails. Doesn't fail now, needs fixing
Add test for evaluation of 0
JuaniRios
changed the title
…ion tests need fixing
…nd-tests # Conflicts: # pallets/funding/src/functions.rs # pallets/funding/src/instantiator.rs # pallets/funding/src/tests/3_auction.rs
vstam1
left a comment
vstam1
left a comment
There was a problem hiding this comment.
Good first changes! Did an review on tests, but will do another on maybe missing paths.
| if plmc_bond_returned > T::ExistentialDeposit::get() { | ||
| T::NativeCurrency::release( | ||
| &HoldReason::Participation(project_id).into(), | ||
| &bid.bidder, | ||
| plmc_bond_returned, | ||
| Precision::Exact, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Why does a release of a hold fail? We are not actually sending tokens, right? Just releasing tokens that are already in the users account?
There was a problem hiding this comment.
As far as I understand from a quick test, its not possible to hold funds if it would put your free balance under ED. So I think this situation should in theory not happen. Lets say we have 100 tokens with ED = 1:
- We hold 100. -> Error
- We hold 90 -> transfer_allow_death(10) -> Error.
So balance should always contain at least ED free tokens in this case.
There was a problem hiding this comment.
This is the logic for placing a hold. It is called with Protect and Force.
The case for erroring out when there's no ED left after hold is when there is only 1 provider. And if I understand correctly, we could in the future introduce pallets that provide for the existence of the account, making it possible for it to live without an ED.
Therefore I prefer to be safe and do a check before releasing so we don't halt the whole project. I don't think most people will be getting a return below ED anyway, and if they do they shouldn't mind it not being returned.
@lrazovic any thoughts?
There was a problem hiding this comment.
For reference here is the logic for releasing an asset. It always checks the amount free + the released amount will be bigger than ED.
There was a problem hiding this comment.
From: paritytech/substrate#12951
"... may be replaced with
fungible::hold::Mutate::release, however the returned Result must be handled and the inner ofOkhas the opposite meaning (i.e. it is the amount released, rather than the amount not released, if any)."
So I don't think that release is infallible. Now I throw the question back at you: is this the best way to handle this possible failure? If yes, then we can resolve the comment.
Tagging also @joepetrowski who maybe has some extra input on this.
There was a problem hiding this comment.
If it fails due to ED, we can cover it. If it fails for some other reason we probably don't want to continue the project transition because it's unexpected behavior and we should fix it and then force run the community start.
So I think we can resolve as is
…rehensive-community-round-tests # Conflicts: # pallets/funding/src/functions.rs # pallets/funding/src/instantiator.rs # pallets/funding/src/tests/3_auction.rs # pallets/funding/src/tests/4_community.rs
vstam1
left a comment
vstam1
left a comment
There was a problem hiding this comment.
Looks good for a couple of last things
JuaniRios
deleted the
feature/plmc-518-comprehensive-community-round-tests
branch
3 participants
What?
How?
Testing?
cargo test --features runtime-benchmarksAnything else?
check https://linear.app/polimec/issue/PLMC-502/community for documentation on all paths covered