-
Notifications
You must be signed in to change notification settings - Fork 12
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
invariant tests #162
invariant tests #162
Conversation
@@ -17,12 +17,15 @@ contract OptionSettlementEngineInvariantTest is BaseEngineTest, InvariantTest { | |||
ProtocolAdmin internal admin; | |||
Timekeeper internal timekeeper; | |||
|
|||
uint256[] internal optionTypes; |
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.
Really nice. I see how we'll be able to track what we need to, and how we can calculate invariants using totals and other variables which may not be tracked by Engine.
We might want to refactor out these arrays into a World object, instead of the test contract itself. This will give us flexibility to use in other test harnesses and it will remove a circular dependency — we would pass the World to each Actor instead of the test suite and include it as a state variable in the test contract (kinda like Timekeeper).
LMK what you think, I'm sure there are trade-offs. Separating out the tracking / accumulating into a world object is a pattern from BDD style testing that I think fits what you're doing well and may open doors for us for some other good techniques too.
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.
agree with the idea, was mostly just working around for the sake of this PR, and I think that would be a good refactor.
but how much more test code are we going to be authoring which would be able to leverage your proposed structure? if the answer is "a lot" then we should definitely hit the refac. but, are we actually going to be writing new test harnesses at this point? should we really invest more hours into refactoring test code?
I'm skeptical that whatever additional tests we're going to write (or not write) would justify spending more time on refactors. The erc20 wrappers seem like a much higher priority.
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.
Sounds good, let's hold on World for time being. Maybe will be helpful for Wrapper, Margin, Vaults, etc. Let's wait and see.
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.
I think the world pattern is not a bad idea, but don't feel strongly that it has to be in this PR. We do need to get this compiling, though.
Codecov Report
@@ Coverage Diff @@
## audit-fixes #162 +/- ##
============================================
Coverage 91.22% 91.22%
============================================
Files 2 2
Lines 376 376
Branches 57 57
============================================
Hits 343 343
Misses 27 27
Partials 6 6 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@Flip-Liquid a follow on PR to the litepaper with some of these invariants expressed mathematically would be useful. |
Nice work @Flip-Liquid We could do an I am still confused why no logs show other than from |
-Add initial invariants for actors