Implemented expiration for cw1-subkeys contract #356
Conversation
hashedone
force-pushed
the
subkeys-allowance-expire
branch
from
e32b6b1 to
441b52c
Compare
| @@ -107,6 +107,10 @@ where | |||
| }) => { | |||
| ALLOWANCES.update::<_, ContractError>(deps.storage, &info.sender, |allow| { | |||
| let mut allowance = allow.ok_or(ContractError::NoAllowance {})?; | |||
| if is_expired(&env, allowance.expires) { | |||
There was a problem hiding this comment.
Wow, super important bug here. 💯
| @@ -127,6 +131,14 @@ where | |||
| Ok(res) | |||
| } | |||
|
|
|||
| pub fn is_expired(env: &Env, expire: Expiration) -> bool { | |||
There was a problem hiding this comment.
No need to implement this, you can do:
expire.is_expired(&env.block)
https://github.com/CosmWasm/cosmwasm-plus/blob/main/packages/cw0/src/expiration.rs#L41-L47
| let mut allowance = allow.unwrap_or_default(); | ||
| ALLOWANCES.update::<_, ContractError>(deps.storage, &spender_addr, |allow| { | ||
| let mut allowance = allow | ||
| .filter(|allow| !is_expired(&env, allow.expires)) |
There was a problem hiding this comment.
Good checks for expiration (and good error below).
I just request using the Expiration method
| @@ -217,7 +239,7 @@ where | |||
|
|
|||
| pub fn execute_decrease_allowance<T>( | |||
| deps: DepsMut, | |||
| _env: Env, | |||
| env: Env, | |||
There was a problem hiding this comment.
Ditto.
(All these _env were good tips we were not checking for expiration.
| // we can use unchecked here as it is a query - bad value means a miss, we never write it | ||
| let spender = deps.api.addr_validate(&spender)?; | ||
| let allow = ALLOWANCES | ||
| .may_load(deps.storage, &spender)? | ||
| .filter(|allow| !is_expired(&env, allow.expires)) |
| @@ -351,7 +391,9 @@ fn can_execute(deps: Deps, sender: String, msg: CosmosMsg) -> StdResult<bool> { | |||
| let allowance = ALLOWANCES.may_load(deps.storage, &sender)?; | |||
| match allowance { | |||
| // if there is an allowance, we subtract the requested amount to ensure it is covered (error on underflow) | |||
| Some(allow) => Ok(allow.balance.sub(amount).is_ok()), | |||
| Some(allow) => { | |||
| Ok(!is_expired(&env, allow.expires) && allow.balance.sub(amount).is_ok()) | |||
| @@ -475,6 +526,14 @@ mod tests { | |||
| withdraw: false, | |||
| }; | |||
|
|
|||
| // Expiration constant working properly with default `mock_env` | |||
| const NON_EXPIRED_HEIGHT: Expiration = Expiration::AtHeight(22_222); | |||
There was a problem hiding this comment.
nice to make these variables.
| assert_eq!(allowance, Allowance::default()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_expired() { |
| } | ||
| ] | ||
| } | ||
| .canonical(), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn previous_expired() { |
There was a problem hiding this comment.
Nice test, it removes previously expired when it add the new one.
| .expire_allowances(SPENDER1, NON_EXPIRED_HEIGHT) | ||
| .init(); | ||
|
|
||
| execute( |
There was a problem hiding this comment.
I would usually asset which error it was. Something like:
let err = execute(...).unwrap_err();
assert!(matches!(err, ContractError::SettingExpiredAllowance(_)), "{}", err);
```…1-subkeys contract
|
I just somehow missed |
…cking for particular error in test
There is one edge case which is not clean - what to do, when previous allowance is expired, but no new allowance is set. There are two reasonable solutions:
IncreaseAllowancein such case would just set expiration toExpire::Never {}as it is default expiration for new allowances. This feels natural, but might cause dangerous usecase: if transaction was scheduled before previous allowance expired, then probably he expected it not to change, but it would change, if transaction would be validated to late (and it would be never expiring)This obviously doesn't impact decreasing allowance, as decreasing non existing allowance is always an error (nothing to decrease).