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

Notional trade module adjustments post audit #256

Conversation

ckoopmann
Copy link
Contributor

@ckoopmann ckoopmann commented Jun 26, 2022

Code Review Processes

New Feature Review

Before submitting a pull request for new review, make sure the following is done:

  • Design doc is created and posted here
  • Code cleanliness and completeness is addressed via guidelines

README Checks

  • [] README has proper context for the reviewer to understand what the code includes, any important design considerations, and areas to pay more attention to

Code Checks

  • Add explanatory comments. If there is complex code that requires specific context or understanding, note that in a comment
  • Remove unncessary comments. Any comments that do not add additional context, information, etc. should be removed
  • Add javadocs.
  • [] Scrub through the code for inconsistencies (e.g. removing extra spaces)
  • [] Ensure there are not any .onlys in spec files

Broader Considerations

  • Ensure variable, function and event naming is clear, consistent, and reflective for the scope of the code.
  • Consider if certain pieces of logic should be placed in a different library, module

@ckoopmann ckoopmann marked this pull request as draft June 26, 2022 03:11
@ckoopmann ckoopmann changed the title Christian/notional trade module adjustments post audit Notional trade module adjustments post audit Jun 27, 2022
@ckoopmann
Copy link
Contributor Author

ckoopmann commented Jun 27, 2022

Summary of Changes:

Adjust Interface for trade methods:

  1. Change trading parameters (token amounts) in relative terms (to the total set supply) rather than in absolute/notional amounts.
  2. Add methods to allow fixed-output-redemptions and fixed-input-mints (basically not fixing the fCash amount but the underlying / asset token instead) and rename existing methods to differentiate them.

Address issues raised in codearena audit:

  1. Add (configurable) gas limit in external function call to getDecodedID inside isWrappedFCash to avoid wasting excessive amounts of gas. (For example fallback method of cEth token wasted all of the gas leading to revertions)

  2. Reset allowances to zero after minting / redeeming to avoid leftover allowances. (and potential revertions on tokens which don't allow changing non-zero allowances such as USDT)

  3. Allow manager to disable automatic redemption of matured positions upon issuance / redemption to avoid persistent revertions in external calls from freezing user funds.

  4. Add _safeUint88 function to avoid silent overflows when downcasting uint256 fCash amounts to uint88 datatype expected by wrappedfCash

  5. Various smaller changes based on QA / gas optimization feedback from the audit contest. (typos, natspec comments etc)

Repo Cleanup:

Remove manual deployment of wFCashFactory etc. in integration tests and related imports etc.. Instead use factory instance deployed by the notional team.

@ckoopmann ckoopmann marked this pull request as ready for review June 27, 2022 02:10
test/integration/curveExchangeALM.spec.ts Outdated Show resolved Hide resolved
test/integration/curveExchangeALM.spec.ts Outdated Show resolved Hide resolved
test/integration/curveExchangeALM.spec.ts Outdated Show resolved Hide resolved
test/integration/curveExchangeALM.spec.ts Show resolved Hide resolved
@ckoopmann
Copy link
Contributor Author

Replaced by: IndexCoop#1

@ckoopmann ckoopmann closed this Oct 1, 2022
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