Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix(constructor): add parameter bounds & extract constants / wExp #66
fix(constructor): add parameter bounds & extract constants / wExp #66
Changes from 6 commits
b44ae2c
77acf95
614f4ff
c9a3e20
a8809c2
ae8ab3c
4e3cc2a
808b51f
0f7774c
2b9ad37
f158703
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
not sure if we need a constantLib for the IRM. Everything in the same file is great
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.
Using a file is handy for integrators (ourselves) and having it in an dedicated file avoids having to compile the IRM in case the constants are used by integrators (including ourselves)
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.
This and if we do follow this suggestion then there should be only 2 constants left in this lib and those would almost never be useful to integrators (who cares about the max curve steepness, you only really care about the curve steepness of the IRM you are interacting with)
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.
The same arguments could be used for Blue but still we extracted constants to a file because it's shown to be handy for our test suite alone ; having constants defined in IRM (such as the min/max rates) require to deploy the contract to test things using these bounds... For example what we do in ExpLib but thanks to the constants library we don't need to deploy an IRM to test the upper value of wExp
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'm not saying that we should never extract constants. My point is that min/max rates should really be immutables, and I think that the other constants would almost never be useful
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.
To me the benefit of having constants defined in a dedicated library and being able to access it outside of the IRM outweighs the loss of conciseness (now the IRM depends on 1 more file)
Whether min/max rates should be immutable is another topic that I'm open to discuss in a dedicated issue
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.
opened #96
I think that those topics are not so independent, but I'm fine with the current code in any case
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.
this value was changed because the previous value didn't correspond to the bound from the unbounded wExp
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.
except that the code is correct (just checked)