-
Notifications
You must be signed in to change notification settings - Fork 115
Add a council mint #253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a council mint #253
Conversation
| // Initializes a new mint | ||
| pub fn create_new_council_mint() -> T::MintId { | ||
| let mint_id = <minting::Module<T>>::add_mint(minting::BalanceOf::<T>::zero(), None) | ||
| .expect("Failed to create a mint for the council"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not happy with panic in the runtime. Can we put mint only on successful creation and during the setting mint parameters - introduce another "ensure_*"? check on whether mint actually exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a fair point. I originally had this expect() inside the build() method before refactoring, where it was okay to panic while constructing genesis.
Given that add_mint can fail and we call it in the migration, I will have to make the mint an Option<MintId>
Let me revise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to make the mint an Option so it doesn't need to be created at genesis and we can avoid panic. The mint can be automatically created when setting capacity if it doesn't exist.
| // ... | ||
|
|
||
| // Create the Council mint. If it fails, we can't do anything about it here. | ||
| let _ = governance::council::Module::<T>::create_new_council_mint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed to print an error message in such cases. Don't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a6148b9
|
Single error message comment. Will be fixed later. LGTM |
Implements #242 and #244
initialized at genesis