pallet_revive: Allow collecting fees from the txpayment hold instead of free_balance #9680
Conversation
athei
changed the title
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| /// Set to `Some` in order to collect all storage deposits from the specified hold. | ||
| /// | ||
| /// If `None` the deposits are collected from free balance. In any case, they are | ||
| /// collected from the transaction signers native balance. |
There was a problem hiding this comment.
is this a feature flag that we will remove later? Not sure why it's an Option otherwise
There was a problem hiding this comment.
I would love to make it non optional. However, this
#[frame_support::register_default_impl(TestDefaultConfig)]forces me to provide a default. Can I skip some types there?
In the mean time I decided to abuse as feature flag. But eventually it shouldn't be optional.
| /// `from` is usually the transaction origin and `to` a contract or | ||
| /// the pallets own account. | ||
| fn charge_deposit( | ||
| hold_reason: Option<HoldReason>, |
There was a problem hiding this comment.
this is always HoldReason::StorageDepositReserve when Some right?
not sure it's not easier to read if we just pass a bool
There was a problem hiding this comment.
Can also be CodeUploadDepositReserve.
substrate/frame/revive/src/lib.rs
Outdated
| T::Currency::transfer_on_hold( | ||
| &deposit_source, | ||
| from, | ||
| to, | ||
| amount, | ||
| Precision::BestEffort, | ||
| Restriction::Free, | ||
| Fortitude::Polite, | ||
| )?; | ||
| if let Some(hold_reason) = hold_reason { | ||
| T::Currency::hold(&hold_reason.into(), to, amount)?; | ||
| } | ||
| }, |
There was a problem hiding this comment.
shouldn't tis be Precision::Exact here? or you can end up transferring some of the free balance of the contract to to the hold
too bad we can't transfer from one hold to the other directly, should we add that feature?`
There was a problem hiding this comment.
shouldn't tis be Precision::Exact here? or you can end up transferring some of the free balance of the contract to to the hold
Got catch. Copy paste fuckup.
too bad we can't transfer from one hold to the other directly, should we add that feature?
You can with MutateHold::transfer_on_hold but it doesn't let you specify the target hold reason. I guess it uses the sorce hold reason. Yes, I think we should add this feature. But changing the trait will be a big refactor. So I decided to not do that here.
Co-authored-by: PG Herveou <[email protected]>
|
Okay the CI fails because is doesn't work with the current gas mapping. This is because there is not enough balance in the hold since we do not pre-charge the deposit as weight. So I will disable the collection from the hold in every runtime. Otherwise we cannot merge to master. |
This builds on #9590. The plan is to get that in first and then this PR. After that new new gas mapping can get id of the the
max_deposit.Is basically just changes were to collect the deposit from depending on configutation and if its an eth or native transaction.
Sorry for tbe big diff but needed to another arg to
bare_calland its always a huge hassle to so. While I had to do this anyways I grouped the all the together in a newExecConfig. Will making new config params much easier, in the future.It will take the storage deposits from he hold created by the transaction payment. It is disabled on Westend for now.