Panic button internal #384
Conversation
programs/marginfi/src/instructions/marginfi_group/panic_unpause_permissionless.rs
Outdated
Show resolved
Hide resolved
|
Closes #339 |
programs/marginfi/src/instructions/marginfi_group/panic_pause.rs
Outdated
Show resolved
Hide resolved
|
|
||
| use crate::state::panic_state::PanicStateImpl; | ||
|
|
||
| pub fn panic_unpause(ctx: Context<PanicUnpause>) -> Result<()> { |
There was a problem hiding this comment.
why even bother having an unpause if it only pauses for 15 minutes?
There was a problem hiding this comment.
Up to 30 with extensions. I suppose we don't strictly need it but a 30 minute pause could be very annoying to the consumers so ideally we make it as short as possible whenever needed.
programs/marginfi/src/instructions/marginfi_group/panic_unpause_permissionless.rs
Show resolved
Hide resolved
|
As mentioned before, it would be nice to see a validation system that e.g. in this case, by default disallows all instructions from interacting with a group in a paused state. Instead, being explicit about which instructions are allowed to be called when the protocol is paused. |
| await banksClient.processTransaction(tx); | ||
| }); | ||
|
|
||
| // Note: This is an interesting edge case to consider. While liquidations are allowed to continue |
There was a problem hiding this comment.
@Henry-E @IliaZyrin Thoughts here before we merge?
Should we allow receivership liquidation to bypass the pause?
I will likely allow the receivership liquidations to bypass the check, which consequently either complicates the check in the constraint that we have here or forces it to move to the handler.
There was a problem hiding this comment.
I think either both liquidations should be allowed or both - disallowed. We chose the former for the "classic" liquidation, so let's just do the same for receivership.
There was a problem hiding this comment.
To be honest I don't quite understand well enough what this pause is trying to achieve and context around it to determine which liquidations should be permitted.
…hey aren't copy-pasta of each other but oh well.
|
|
||
| // If already paused and not expired, treats this as an "extend" operation. | ||
| if self.is_paused_flag() && !self.is_expired(current_timestamp) { | ||
| self.pause_start_timestamp = self |
There was a problem hiding this comment.
How does this work exactly? Let's say at N+x where x<pause_duration if we shift forward the pause start timestamp from N to N+pause_duration. I'm assuming that this doesn't deactivate the pause somehow from between N+x -> N+pause_duration?
There was a problem hiding this comment.
nit - Perhaps this is one reason using a pause_start_timestamp is less intuitive than using an end_timestamp
There was a problem hiding this comment.
That's the goal, and for you to validate in your review! It does have the awkward side effect of start being in the future, which is indeed somewhat less intuitive.
4 participants
See #381 for description
Thanks for FEMI for the assist.
Changes: