Repository navigation
Conversation
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
just wanted to look at the behavior if below minimum rent balance, vyr minor thing. good otherwise.
| .checked_sub(Rent::get()?.try_minimum_balance(native_sol_buffer.data_len())?) | ||
| // Basically unreachable: a live buffer is rent-exempt unless the rent | ||
| // mechanism itself changed under it. | ||
| .ok_or(ProgramError::AccountNotRentExempt)?; |
There was a problem hiding this comment.
minor: since we expect the code to be unreachable anyway, you could say that the only way this could happen is if some protocol upgrade happens in the future. For example, if the cost of rent is raised without requiring older accounts to immediately meet the new rent exempt cap.
in this case, it would be better behavior to simply cap to 0, right? that way the automated script that withdraws funds doesn't suddenly stop working aggressively just because of a protocol upgrade.
In thi particular case it does not seem to be an issue because more funds for rent can simply be deposited into the account, but we should be careful when making assumptions about the rent rules.
There was a problem hiding this comment.
In general I prefer explicit errors, but I agree there are more benefits here to avoid errors in the automated withdraw script.
e096295
| // Basically unreachable: a live buffer is rent-exempt unless the rent | ||
| // mechanism itself changed under it. | ||
| .ok_or(ProgramError::AccountNotRentExempt)?; | ||
| let amount = amount.min(withdrawable); |
There was a problem hiding this comment.
its a little wierd to me that we don't just make amount mutable from the first definition. Guess it basically matters nil, though.
There was a problem hiding this comment.
In general it's good practice to avoid mut so that all mutations of a specific variable are seen explicitly in the code. Not too important here though.
|
|
||
| #[test] | ||
| fn process_withdraw_native_sol_rejects_draining_the_buffer() { | ||
| assert_rejects( | ||
| base_accounts(), | ||
| BUFFER_LAMPORTS, | ||
| SettlementError::NativeSolBufferEmptied.into(), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn process_withdraw_native_sol_rejects_more_than_the_balance() { | ||
| assert_rejects( | ||
| base_accounts(), | ||
| BUFFER_LAMPORTS + 1, | ||
| ProgramError::ArithmeticOverflow, | ||
| ); | ||
| } |
There was a problem hiding this comment.
i noticed we removed a few optentially useful tests due to adding the use fo the Rent opcode. I am generally OK with this, and I am not expecting anything I say here to change anything, but I stillu think we should reevaluate our policy on not overriding for purposes of unit testing the sysvars so we can properly unit tests. the benefits are large.
There was a problem hiding this comment.
I agree this isn't nice, but I don't see a clear fix here, it's more of a generic Solana problem with unit testing program code.
250b83a
into
kaze/sc-403-withdraw-sol-from-native-buffer
2 participants
Implements suggestion from this comment to silently cap the amount withdrawn in a
WithdrawNativeSolinstruction to the full balance minus the rent. This is done so that a script that automatically withdraws the fees won't occasionally revert if at the same time a settlement is using funds from the native SOL buffer.How to test
Existing and modified tests.