-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate allowance expiration #792
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, just one suggestion.
Nice tests!
let mut val = allow.unwrap_or_default(); | ||
if let Some(exp) = expires { | ||
if exp.is_expired(&env.block) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You found this helper instead of writing the check yourself. Nice!
let mut val = allow.unwrap_or_default(); | ||
if let Some(exp) = expires { | ||
if exp.is_expired(&env.block) { | ||
return Err(ContractError::Expired {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More a suggestion than a blocker. I think what we want is more a validation error, just to be clearer to the user/consumer. The problem isn't that an existing allowance is expired, the problem is the user is trying to create/modify an allowance so that it expires in the past.
I'd add a new error variant here:
https://github.com/CosmWasm/cw-plus/blob/main/contracts/cw20-base/src/error.rs
For some reason CircleCI is not picking up this branch. Let's figure that out... |
Closing this, as it is a part of #793 and the ci picked that one up. |
Closes #628
Adds a check for valid expiration when increasing or decreasing allowance.
Also had to change some of the tests because they were creating expired allowances to test the expiration checks of the other message handlers.