Skip to content
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

More test coverage on sad paths #94

Merged
merged 5 commits into from
Nov 10, 2022
Merged

Conversation

neodaoist
Copy link
Contributor

@neodaoist neodaoist commented Nov 7, 2022

Adds/updates unit tests for error conditions that weren't being covered. There are a 4 code paths / errors I think may be unreachable. Would love feedback, and if so, I can either remove the check/error itself or update the test to exercise the sad path.

  1. write() L227 — how can a user hit InvalidOption revert?

  2. write() L263 — how can a user hit AlreadyClaimed revert? In other words, how can a user redeem a claim (when now >= expiry) and then write new options to the same contract, when we can't add new options to an expired contract?

  3. redeem() L353 — how can a user have balance != 0 of a claim which is burned? I think this would have to be true for a call to reach this code path, otherwise it's caught during the if (balance != 1) { revert CallerDoesNotOwnClaimId(claimId); } check. Look at the commented-out test on L1177 — in this case, the claim NFT which would have balance == 1 from the 2nd write() call is not the claimId being passed to the 2nd redeem() call. Depending if number 2 and number 3 are actually unreachable, the AlreadyClaimed() error would not be thrown anywhere.

  4. NoClaims error is not being thrown anywhere anymore (only instance I see is a previous usage in valorem-options/lib/valorem-core) — good to remove, or do we need to add a check to exercise() for the case where a user tries to redeem a claim for an option type with no options written, or outstanding ?

@0xAlcibiades
Copy link
Member

@neodaoist we need to resolve merge conflicts here.

@neodaoist
Copy link
Contributor Author

neodaoist commented Nov 8, 2022

Merge conflicts resolved, but before merging this PR I'd like to address the 4 points above

@0xAlcibiades
Copy link
Member

#97 - I have broken out the concerns into an issue. We can triage all issues on Monday.

@0xAlcibiades 0xAlcibiades merged commit fafa5ac into master Nov 10, 2022
@0xAlcibiades 0xAlcibiades deleted the feature/coverage-on-reverts branch November 10, 2022 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants