Insrt-Finance / insrt-v2-contracts

0 stars 3 forks source link

State Machines for Mint Attempts & Resolutions #121

Closed CruzMolina closed 1 year ago

CruzMolina commented 1 year ago

We probably should consider implementing a state machine per collection so that depositors can't game the system and minting is actually provably fair.

This will likely entail setting a locking state (paused?) for a collection when any mints are pending, and an "open", "green light", or "available" state when there are no active mints waiting to be resolved for a collection.

NouDaimon commented 1 year ago

If minters are minting in rapid succession, depositors will be unable to withdraw - this is an edge case, and one where I doubt it would happen unless PMF is massive.

The locking mechanism will probably have to rely on some enumerable list like: mapping(address collection => EnumerableSet.UintSet requestIds) activeRequestIds;

or array:

mapping(address collection => uint256[] requestIds) activeRequestIds;

and be some condition like:

if(requestIds.length != 0) { revert CollectionHasActiveMints(); }

with an entry being removed upon resolution of the mint, as a final state change.

It must be for all active mints, otherwise it seems moot.

NouDaimon commented 1 year ago

Agree this must be solve. That's the first thought that comes to mind - keen to discuss further.

Could flag for V2, potentially.

CruzMolina commented 1 year ago
mapping(address collection => EnumerableSet.UintSet requestIds) activeRequestIds;

yup, i think going with this makes the most sense to make our lives easier. also agree that it should be for all active mints for a given collection. a secondary effect to think about is that adding this type of pattern probably means it's even more important to figure out sane guards for depositors and tokens per collection because if a mint resolution fails then depositors are effectively locked in unless we override.

it's a pain to think about but i'm wondering if we should make minting pausable to protect minters if the protocol gets backed up (failed mint resolutions), and/or some type of depositor idle/withdraw override so that once minting is paused, and we are able to simulate failed fulfillment tx's and see that a depositor or token is not selected, we can allow depositors to exit as a failsafe while we sort out the mess.

NouDaimon commented 1 year ago

adding this type of pattern probably means it's even more important to figure out sane guards for depositors and tokens per collection because if a mint resolution fails then depositors are effectively locked in unless we override

Agree 100%. Can only figure that limit out after this issue is resolved.

we can allow depositors to exit as a failsafe while we sort out the mess.

Chills down my spine - hopefully we will never be in a situation like this.

wondering if we should make minting pausable

It makes sense to have a pausable guard here tbh - in essence minting is the "engine" of the protocol. We should perhaps add the same to deposits, but it doesn't impact nearly as much.

I think now makes the most sense to do the refactoring, given all the issues which seem to be touching it.

NouDaimon commented 1 year ago

@CruzMolina This seems more nuanced than I initially thought. Similar edge cases may be found for updating token risks or idling tokens.

The trouble is that if updating a token risk is guarded by the state-machine, it means that depositors may be locked into a perpetual inability to withdraw their tokens or change their risk, if minters are really active on that collection, same for withdrawing.

It seems that this may pass the "bad actor" hat from depositors to minters, if the fix is applied in a superficial manner.

I'll think on this a bit more and return here with additional thoughts.

NouDaimon commented 1 year ago

Some initial ideas:

CruzMolina commented 1 year ago

forceWithdraw or some parameter in these functions to permit altering the risk-state with a penalty (forfeit x amount of earnings)

like the idea but this still seems game-able

condition check that if X mins have passed since last attempt to withdraw/update/idle, it is fine to perform the action, and once the action is performed set that state back to its initial - in essence a secondary state machine.

I like this option the most, with the caveat that if the depositor's token is the last token in the collection pool, they should be blocked from updating or removing risk until all mint requests are fulfilled.

cyclical permission. Every x amount of time, for a y-length period of time, these actions are permitted even if there are unfulfilled mints.

this feels like we're defining a periodic freebie pass for depositors. i think i like this option the least.

NouDaimon commented 1 year ago

like the idea but this still seems game-able

Could 100% be - really depends on the penalty versus token mechanics dynamics

I like this option the most, with the caveat that if the depositor's token is the last token in the collection pool, they should be blocked from updating or removing risk until all mint requests are fulfilled.

Good catch!

this feels like we're defining a periodic freebie pass for depositors. i think i like this option the least.

I was envisioning some cyclicity in terms of all the actions that could take place - I was trying to get at something similar with the epochs you mentioned. The epochs are better as the separation of actions is enforced whereas in the cyclicity approach it would rely on the information minters posses over the cycles.

NouDaimon commented 1 year ago

Just to make sure we are aligned, I will implement this state machine so that it bars every single "gameable" action aka:

for now.

CruzMolina commented 1 year ago

Just to make sure we are aligned, I will implement this state machine so that it bars every single "gameable" action aka:

  • updateTokenRisk
  • idleToken
  • withdraw

for now.