Refactor lottery pallet to use fungible traits #10045
Conversation
gui1117
left a comment
gui1117
left a comment
There was a problem hiding this comment.
LGTM, but you can remove the migration, I don't see anything to migrate here.
substrate/frame/lottery/src/lib.rs
Outdated
| &winner, | ||
| lottery_balance, | ||
| KeepAlive, | ||
| Preservation::Preserve, |
There was a problem hiding this comment.
I think the intention is more a transfer_all instead of manually getting the balance and transferring it.
Then you could also remove the pot function.
There was a problem hiding this comment.
Yeah good call !
I also had a question regarding the pallet account.
Technically speaking if we dont want the pallet account to be deleted it needs to have ED right ? or is there some specificity to this that makes it ignore ED ?
Cause if it needs ED then either we need to give him ED from the start or reduce the first pot by ED amount to compensate.
There was a problem hiding this comment.
Unless i am mistaken I dont think there is currently a transfer_all type function in fungibles mutate
and the transfer_all function from pallet_balances does a pretty similar logic. I just need to be sure about the ED situation with the internal account to know if i keep alive or not.
|
|
||
| assert_eq!(Balances::free_balance(&1), 100); | ||
| assert_eq!( | ||
| Balances::reducible_balance(&1, Preservation::Expendable, Fortitude::Polite), |
There was a problem hiding this comment.
Can you check that the payout still works when there is an additional consumer, provider or sufficient reference on the account?
Just to check that nobody can block the payout i this case.
There was a problem hiding this comment.
Thanks for this comment ! I had no idea of the existance of these so now i understand that you can technically have an account doesnt get reaped even tho he doesnt have ED !
Also do you mean adding a reference/consumer to the pallet account or the winner's account ?
I dont see how it would be an issue on the winner but i can see how it can be on the pallet account where the funds are held.
With all these info i have a new question !
For the sake of optimization (atm we delete the account by draining it of its funds and then recreating and minting so ED in it every lottery) wouldnt it be better to just give this lottery account some sufficient reference ? that way no need to detroy/mint again everytime ... that could even help in indexing or some external monitoring.
|
/cmd label T2-pallets |
|
/cmd fmt |
paritytech-cmd-bot-polkadot-sdk
bot
added
the
T2-pallets
|
/cmd bench |
|
Command "bench" has started 🚀 See logs here |
|
/cmd bench --pallet pallet_lottery --runtime kitchensink-runtime |
|
@Krayt78 still misses a prdoc. |
|
Command "bench --pallet pallet_lottery --runtime kitchensink-runtime" has started 🚀 See logs here |
|
Command "bench --pallet pallet_lottery --runtime kitchensink-runtime" has failed ❌! See logs here |
|
/cmd prdoc |
substrate/frame/lottery/src/lib.rs
Outdated
| if frame_system::Pallet::<T>::sufficients(&lottery_account) == 0 { | ||
| frame_system::Pallet::<T>::inc_sufficients(&lottery_account); |
There was a problem hiding this comment.
i don't understand why you increment only when sufficient is 0.
I would think this pallet add a reference counter when the lottery is started, and removes it when it is finished.
Otherwise, you do not know when that sufficient reference counter would be added or removed, since it is managed by other runtime logic
There was a problem hiding this comment.
misunderstanding+ oversight on my part ! Thanks Shawn :)
There was a problem hiding this comment.
looking closer to your intention, if the goal is to keep the lottery account alive, perhaps reference counters are not the right approach. Treasury uses logic like:
https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/treasury/src/lib.rs#L999
Which ensures the lottery account is always in a non-zero balance.
Also using transfer functions like keep alive make sense to me.
I am not quite sure what @ggwpez is looking for with regard to behaviors with the reference counters.
…ely. Added cleanup for sufficient references when the lottery ends and streamlined the logic for incrementing sufficient references on lottery start.
Context from a discussion with @shawntabrizi regarding this crateI am exploring ways to keep the pallet’s account alive in order to avoid triggering account creation and deletion on every lottery round. Previously, this was handled by minting the Existential Deposit (ED) into the account whenever its balance dropped below ED. This approach can be problematic on chains with a fixed or limited token supply. This leaves us with two main alternatives:
At the moment, this is a tradeoff between system cleanliness and simplicity, and there is no single “perfect” solution, only different design choices with different drawbacks. would love your opinion on the matter @ggwpez ! |
5 participants
This PR migrates the lottery pallet from the Currency trait to the fungible traits
There doesn't seem to be a need for runtime migration
This is part of the umbrella task #226