timewave-computer / covenants

Apache License 2.0
17 stars 4 forks source link

liquid staker fsm cleanup #282

Closed bekauz closed 1 month ago

bekauz commented 1 month ago

closes #278

also adds non-std errors for the contract.

curious what are the thoughts on something like this:

    #[error("State machine error: cannot perform {0:?} from {1:?} state")]
    StateMachineError(ExecuteMsg, ContractState),

which would be used in a way similar to this:

match (CONTRACT_STATE.load(deps.storage)?, msg) {
        < ... >
        // in order to perform the transfer, ICA needs to be created
        (ContractState::Instantiated, ExecuteMsg::Transfer { amount }) => {
            Err(ContractError::StateMachineError(
                ExecuteMsg::Transfer { amount },
                ContractState::Instantiated,
            )
            .into())
        }
    }

I think it may be nice to generalize this state machine error type for all FSM-based contracts. We could have a dedicated fsm error trait for imposing a specific format that would return error messages along the lines of "cannot perform from ". Thoughts?

stiiifff commented 1 month ago

Why not extend the ContractOperationError (i.e. with an InvalidOperation) vs adding a new error type ?

stiiifff commented 1 month ago

I like the idea of generalizing, and maybe having a base FSM contract, so that we implement all our contract in the same way, with a very intuitive and predictable interface.

bekauz commented 1 month ago

makes sense to me but it's not obvious how that generalization would work, especially when the FSM logic is being rewritten with these issues to be honest.

regarding ContractOperationError, i'll make an issue to revisit this towards the end of this release ok? just wanted to be sure that we don't bring in some error variants into contracts that shouldn't deal with them. for instance a native router will be dealing with be dealing with op_mode but not with state machine transitions.

stiiifff commented 1 month ago

@bekauz LGTM