Draft
Conversation
feat: maxSupplyCap minter vault
…ply cap and request redeemer liquidity handling
…equest instructions
feat: add safe approval methods for mint and redeem requests with sup…
Signed-off-by: kostyamospan <37243242+kostyamospan@users.noreply.github.com>
Summary of ChangesHello @kostyamospan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Midas Vaults protocol by introducing a maximum supply cap for mTokens, providing more robust control over token issuance. It also refactors and expands the approval mechanisms for both mint and redeem requests with new 'safe' instructions, offering greater flexibility and error handling. Furthermore, the changes include improvements to data feed management and a comprehensive set of new developer tools for better monitoring and verification of the system's state. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant features, including a max supply cap for mTokens, safer approval flows for mint/redeem requests with options to skip on certain conditions, and substantial refactoring of scripts and tests. The changes are extensive and well-structured. My review focuses on ensuring the new logic is sound and doesn't introduce vulnerabilities. I've identified a critical issue in the order of operations for redeem approvals that could lead to fund loss, along with several potential panic points in the new utility functions due to the use of unwrap(). I've also suggested some simplifications for improved code clarity.
programs/midas-vault/src/utils.rs
Outdated
Comment on lines
+692
to
+698
| let amount_to_mint = (request.deposited_usd_wo_fees as u128) | ||
| .checked_mul(ONE.into()) | ||
| .unwrap() | ||
| .checked_div(new_out_rate) | ||
| .unwrap(); | ||
|
|
||
| if !validate_max_supply_cap(m_mint, minter_vault, amount_to_mint.try_into().unwrap()) { |
There was a problem hiding this comment.
This function uses .unwrap() in two places, which can cause the program to panic:
checked_div(new_out_rate).unwrap(): Ifnew_out_rateis 0, this will panic. An admin could provide 0 as an argument.amount_to_mint.try_into().unwrap(): This converts au128to au64. Ifamount_to_mintexceedsu64::MAX, this will panic.
These should be handled by returning a Result::Err instead of panicking to make the program more robust.
let amount_to_mint = (request.deposited_usd_wo_fees as u128)
.checked_mul(ONE.into())
.ok_or(MidasVaultsError::InvalidOutAmount)?
.checked_div(new_out_rate)
.ok_or(MidasVaultsError::InvalidRate)?;
let amount_to_mint_u64: u64 = amount_to_mint.try_into().map_err(|_| MidasVaultsError::InvalidOutAmount)?;
if !validate_max_supply_cap(m_mint, minter_vault, amount_to_mint_u64) {
programs/midas-vault/src/utils.rs
Outdated
Comment on lines
+235
to
+238
| pub fn validate_max_supply_cap(m_mint: &Mint, minter: &MinterVaultState, mint_amount: u64) -> bool { | ||
| let new_supply = m_mint.supply.checked_add(mint_amount).unwrap(); | ||
| minter.max_supply_cap >= new_supply | ||
| } |
There was a problem hiding this comment.
The use of .unwrap() on checked_add can cause a panic if the total supply overflows a u64. While this is an edge case, it's safer to handle potential overflows gracefully by returning false instead of panicking.
Suggested change
| pub fn validate_max_supply_cap(m_mint: &Mint, minter: &MinterVaultState, mint_amount: u64) -> bool { | |
| let new_supply = m_mint.supply.checked_add(mint_amount).unwrap(); | |
| minter.max_supply_cap >= new_supply | |
| } | |
| pub fn validate_max_supply_cap(m_mint: &Mint, minter: &MinterVaultState, mint_amount: u64) -> bool { | |
| match m_mint.supply.checked_add(mint_amount) { | |
| Some(new_supply) => minter.max_supply_cap >= new_supply, | |
| None => false, // Overflow implies the cap is exceeded. | |
| } | |
| } |
Comment on lines
+133
to
156
| match minter::approve_mint_request( | ||
| &ctx.accounts.mint_request, | ||
| &ctx.accounts.vault_common, | ||
| &ctx.accounts.minter_vault, | ||
| &ctx.accounts.m_mint, | ||
| &ctx.accounts.m_mint_user_ata, | ||
| &ctx.accounts.m_mint_token_program, | ||
| &ctx.accounts.user_account, | ||
| &ctx.accounts.token_authority, | ||
| &ctx.accounts.vault_minter_role, | ||
| &ctx.accounts.system_program, | ||
| &ctx.accounts.token_authority_program, | ||
| request_id, | ||
| new_out_rate.into(), | ||
| is_safe, | ||
| skip_on_supply_cap_exceeded, | ||
| ) { | ||
| Ok(true) => { | ||
| ctx.accounts.close()?; | ||
| Ok(()) | ||
| } | ||
| Ok(false) => Ok(()), | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
The match statement for handling the Result<bool> can be simplified for better readability and conciseness. Using if let or the ? operator is more idiomatic in this case.
if minter::approve_mint_request(
&ctx.accounts.mint_request,
&ctx.accounts.vault_common,
&ctx.accounts.minter_vault,
&ctx.accounts.m_mint,
&ctx.accounts.m_mint_user_ata,
&ctx.accounts.m_mint_token_program,
&ctx.accounts.user_account,
&ctx.accounts.token_authority,
&ctx.accounts.vault_minter_role,
&ctx.accounts.system_program,
&ctx.accounts.token_authority_program,
request_id,
new_out_rate.into(),
is_safe,
skip_on_supply_cap_exceeded,
)? {
ctx.accounts.close()?;
}
Ok(())
Comment on lines
+149
to
+172
| match minter::approve_mint_request( | ||
| &ctx.accounts.mint_request, | ||
| &ctx.accounts.vault_common, | ||
| &ctx.accounts.minter_vault, | ||
| &ctx.accounts.m_mint, | ||
| &ctx.accounts.m_mint_user_ata, | ||
| &ctx.accounts.m_mint_token_program, | ||
| &ctx.accounts.user_account, | ||
| &ctx.accounts.token_authority, | ||
| &ctx.accounts.vault_minter_role, | ||
| &ctx.accounts.system_program, | ||
| &ctx.accounts.token_authority_program, | ||
| request_id, | ||
| current_rate, | ||
| true, | ||
| skip_on_supply_cap_exceeded, | ||
| ) { | ||
| Ok(true) => { | ||
| ctx.accounts.close()?; | ||
| Ok(()) | ||
| } | ||
| Ok(false) => Ok(()), | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
The match statement for handling the Result<bool> can be simplified for better readability and conciseness. Using if let or the ? operator is more idiomatic in this case.
if minter::approve_mint_request(
&ctx.accounts.mint_request,
&ctx.accounts.vault_common,
&ctx.accounts.minter_vault,
&ctx.accounts.m_mint,
&ctx.accounts.m_mint_user_ata,
&ctx.accounts.m_mint_token_program,
&ctx.accounts.user_account,
&ctx.accounts.token_authority,
&ctx.accounts.vault_minter_role,
&ctx.accounts.system_program,
&ctx.accounts.token_authority_program,
request_id,
current_rate,
true,
skip_on_supply_cap_exceeded,
)? {
ctx.accounts.close()?;
}
Ok(())
Comment on lines
+131
to
+154
| match minter::approve_mint_request( | ||
| &ctx.accounts.mint_request, | ||
| &ctx.accounts.vault_common, | ||
| &ctx.accounts.minter_vault, | ||
| &ctx.accounts.m_mint, | ||
| &ctx.accounts.m_mint_user_ata, | ||
| &ctx.accounts.m_mint_token_program, | ||
| &ctx.accounts.user_account, | ||
| &ctx.accounts.token_authority, | ||
| &ctx.accounts.vault_minter_role, | ||
| &ctx.accounts.system_program, | ||
| &ctx.accounts.token_authority_program, | ||
| request_id, | ||
| new_out_rate, | ||
| false, | ||
| skip_on_supply_cap_exceeded, | ||
| ) { | ||
| Ok(true) => { | ||
| ctx.accounts.close()?; | ||
| Ok(()) | ||
| } | ||
| Ok(false) => Ok(()), | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
The match statement for handling the Result<bool> can be simplified for better readability and conciseness. Using if let or the ? operator is more idiomatic in this case.
if minter::approve_mint_request(
&ctx.accounts.mint_request,
&ctx.accounts.vault_common,
&ctx.accounts.minter_vault,
&ctx.accounts.m_mint,
&ctx.accounts.m_mint_user_ata,
&ctx.accounts.m_mint_token_program,
&ctx.accounts.user_account,
&ctx.accounts.token_authority,
&ctx.accounts.vault_minter_role,
&ctx.accounts.system_program,
&ctx.accounts.token_authority_program,
request_id,
new_out_rate,
false,
skip_on_supply_cap_exceeded,
)? {
ctx.accounts.close()?;
}
Ok(())
Comment on lines
+170
to
+193
| match redeemer::approve_redeem_request( | ||
| &ctx.accounts.redeem_request, | ||
| &ctx.accounts.vault_common, | ||
| &ctx.accounts.redeemer_vault, | ||
| &ctx.accounts.m_mint_token_program, | ||
| &ctx.accounts.m_mint, | ||
| &ctx.accounts.m_mint_vault_ata, | ||
| &mut ctx.accounts.payment_mint_state, | ||
| Some(&ctx.accounts.payment_mint), | ||
| Some(&ctx.accounts.payment_mint_token_program), | ||
| Some(&ctx.accounts.payment_mint_redeemer_ata), | ||
| Some(&ctx.accounts.payment_mint_user_ata), | ||
| request_id, | ||
| new_m_token_rate.into(), | ||
| true, | ||
| safe_validate_liquidity, | ||
| ) { | ||
| Ok(true) => { | ||
| ctx.accounts.close()?; | ||
| Ok(()) | ||
| } | ||
| Ok(false) => Ok(()), | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
The match statement for handling the Result<bool> can be simplified for better readability and conciseness. Using if let or the ? operator is more idiomatic in this case.
if redeemer::approve_redeem_request(
&ctx.accounts.redeem_request,
&ctx.accounts.vault_common,
&ctx.accounts.redeemer_vault,
&ctx.accounts.m_mint_token_program,
&ctx.accounts.m_mint,
&ctx.accounts.m_mint_vault_ata,
&mut ctx.accounts.payment_mint_state,
Some(&ctx.accounts.payment_mint),
Some(&ctx.accounts.payment_mint_token_program),
Some(&ctx.accounts.payment_mint_redeemer_ata),
Some(&ctx.accounts.payment_mint_user_ata),
request_id,
new_m_token_rate.into(),
true,
safe_validate_liquidity,
)? {
ctx.accounts.close()?;
}
Ok(())
Comment on lines
+151
to
+174
| match redeemer::approve_redeem_request( | ||
| &ctx.accounts.redeem_request, | ||
| &ctx.accounts.vault_common, | ||
| &ctx.accounts.redeemer_vault, | ||
| &ctx.accounts.m_mint_token_program, | ||
| &ctx.accounts.m_mint, | ||
| &ctx.accounts.m_mint_vault_ata, | ||
| &mut ctx.accounts.payment_mint_state, | ||
| Some(&ctx.accounts.payment_mint), | ||
| Some(&ctx.accounts.payment_mint_token_program), | ||
| Some(&ctx.accounts.payment_mint_redeemer_ata), | ||
| Some(&ctx.accounts.payment_mint_user_ata), | ||
| request_id, | ||
| new_m_token_rate.into(), | ||
| false, | ||
| safe_validate_liquidity, | ||
| ) { | ||
| Ok(true) => { | ||
| ctx.accounts.close()?; | ||
| Ok(()) | ||
| } | ||
| Ok(false) => Ok(()), | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
The match statement for handling the Result<bool> can be simplified for better readability and conciseness. Using if let or the ? operator is more idiomatic in this case.
if redeemer::approve_redeem_request(
&ctx.accounts.redeem_request,
&ctx.accounts.vault_common,
&ctx.accounts.redeemer_vault,
&ctx.accounts.m_mint_token_program,
&ctx.accounts.m_mint,
&ctx.accounts.m_mint_vault_ata,
&mut ctx.accounts.payment_mint_state,
Some(&ctx.accounts.payment_mint),
Some(&ctx.accounts.payment_mint_token_program),
Some(&ctx.accounts.payment_mint_redeemer_ata),
Some(&ctx.accounts.payment_mint_user_ata),
request_id,
new_m_token_rate.into(),
false,
safe_validate_liquidity,
)? {
ctx.accounts.close()?;
}
Ok(())
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.