Introduce and Implement VestedTransfer Trait
#5630
Conversation
shawntabrizi
changed the title
VestedTransfer TraitVestedTransfer Trait
|
After this PR, I would like to open a PR to Treasury pallet to enable the option to do a |
shawntabrizi
added
T1-FRAME
substrate/frame/vesting/src/lib.rs
Outdated
| &target, | ||
| schedule.locked(), | ||
| schedule.per_block(), | ||
| schedule.starting_block(), | ||
| ); | ||
| debug_assert!(res.is_ok(), "Failed to add a schedule when we had to succeed."); | ||
| )?; |
There was a problem hiding this comment.
This means that everyone calling this will need to use transactional to revert the changes if there is an error.
There was a problem hiding this comment.
everyone in frame uses transactional? what am I missing?
All code inside the runtime now assume we can error at any point in the code. when this was written, it wasn't the case, hence the need to "double check"
There was a problem hiding this comment.
But VestedTransfer::vested_transfer function is not a pallet call. If someone implements an error recovery logic it can get messy I suppose.
Maybe it is standard practice now, but I would add a small comment on top of do_vesting_transfer that the storage is dirty in case the method becomes public in the future.
And in the doc of VestedTransfer::vested_transfer I would also write that the storage can be dirty when error is returned.
There was a problem hiding this comment.
But
VestedTransfer::vested_transferfunction is not a pallet call. If someone implements an error recovery logic it can get messy I suppose.
Exactly. If someone handles the error, it will not be transactional. Or you do the transactional handling explicitly in the implementation of the trait.
There was a problem hiding this comment.
Thanks, I really didn't consider this. I have explicitly added a transactional layer to the trait implementation.
I feel there must be many traits in Polkadot SDK which are vulnerable to the same problem, right?
There was a problem hiding this comment.
Okay i brought back this check, not looking to cause an issue in Polkadot due to small optimization.
There was a problem hiding this comment.
I agree that in general people should probably rarely recover from a dispatch error, and it would be simpler to require the caller to rollback.
Maybe we can make a transition with a new type: DispatchErrorDirty or DispatchErrorToRollback which means that the storage is dirty and should be rollbacked to handle the error manually.
Or maybe everybody already assume that a DispatchError can have a dirty storage.
There was a problem hiding this comment.
yeah this kind of direction would be nice overall imo
Co-authored-by: Guillaume Thiolliere <[email protected]>
substrate/frame/vesting/src/lib.rs
Outdated
| &target, | ||
| schedule.locked(), | ||
| schedule.per_block(), | ||
| schedule.starting_block(), | ||
| ); | ||
| debug_assert!(res.is_ok(), "Failed to add a schedule when we had to succeed."); | ||
| )?; |
There was a problem hiding this comment.
I agree that in general people should probably rarely recover from a dispatch error, and it would be simpler to require the caller to rollback.
Maybe we can make a transition with a new type: DispatchErrorDirty or DispatchErrorToRollback which means that the storage is dirty and should be rollbacked to handle the error manually.
Or maybe everybody already assume that a DispatchError can have a dirty storage.
Co-authored-by: Guillaume Thiolliere <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR introduces a
VestedTransferTrait, which handles making a transfer while also applying a vesting schedule to that balance.This can be used in pallets like the Treasury pallet, where now we can easily introduce a
vested_spendextrinsic as an alternative to giving all funds up front.We implement
()for theVestedTransfertrait, which just returns an error, and allows anyone to opt out from needing to use or implement this trait.This PR also updates the logic of
do_vested_transferto remove the "pre-check" which was needed before we had a default transactional layer in FRAME.Finally, I also fixed up some bad formatting in the test.rs file.