pallet assets: Fix errors #4118
Conversation
| @@ -491,7 +491,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> { | |||
| let d = Asset::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?; | |||
| ensure!( | |||
| d.status == AssetStatus::Live || d.status == AssetStatus::Frozen, | |||
| Error::<T, I>::AssetNotLive | |||
| Error::<T, I>::IncorrectStatus | |||
There was a problem hiding this comment.
Can't we say AssetNotLiveOrFrozen? Not sure if IncorrectStatus is much helpful.
There was a problem hiding this comment.
I'm personally fine with IncorrectStatus. It's non-ambiguous since it's only used in one context, and allows for us to add new statuses in the future without needing to update the error.
There was a problem hiding this comment.
AssetNotLiveNorFrozen or AssetNotLiveOrFrozen ?
I will do.
There was a problem hiding this comment.
allows for us to add new statuses in the future without needing to update the error.
Yeah, it helps us, but not sure someone using a UI would get any info with the IncorrectStatus error. Either ways, its fine but I would prefer the errors to be more meaningful for the users.
There was a problem hiding this comment.
I don't think Rust enum errors are supposed to be shown directly to end-users in UIs? I always understood them as more for developers.
There was a problem hiding this comment.
Some UI might show the doc: /// The asset status is not the expected status.
Considering there is 3 status I think it is ok
|
@thiolliere could you please add a prdoc? |
|
I updated the minor version of pallet assets, I don't know if it is wanted. |
| Some calls in pallet assets have better errors. No new error is introduced, only more sensible choice are made. | ||
| - audience: Runtime User | ||
| description: | | ||
| Some calls in pallet assets have better errors. No new error is introduced, only more sensible choice are made. |
There was a problem hiding this comment.
I feel information is not useful, but in theory it impacts both dev for maybe test to udpate and frontend.
There was a problem hiding this comment.
in theory it is kind of a major breaking change :-(
a8f4f4f
LiveAssetis an error to be returned when an asset is not supposed to be live.And
AssetNotLiveis an error to be returned when an asset is supposed to be live, I don't think frozen qualifies as live.