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

Wrote APYRescue as well as TokenEnabler contract. Included mainnet fo… #268

Merged
merged 5 commits into from Apr 26, 2023

Conversation

bweick
Copy link
Contributor

@bweick bweick commented Apr 25, 2023

…rking tests of current state to make sure that strategy will be successful.

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: [Insert Link]
  • 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

…rking tests of current state to make sure that strategy will be successful.
Copy link
Contributor

@0xSachinK 0xSachinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two major suggestions (can discuss):

  1. Support Sets with more than one component.
  2. Add a withdraw Set flow.

Also, a few test cases are missing. Let's add those.

contracts/product/APYRescue.sol Outdated Show resolved Hide resolved
contracts/product/APYRescue.sol Outdated Show resolved Hide resolved
test/product/apyRescue.spec.ts Show resolved Hide resolved
});
});

describe("#withdrawRescuedFunds", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing cases: Withdraw before rescue is done.

test/product/tokenEnabler.spec.ts Outdated Show resolved Hide resolved
test/product/tokenEnabler.spec.ts Show resolved Hide resolved
Copy link
Contributor

@0xSachinK 0xSachinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the missing cases in the withdrawRescuedFunds tests. Rest LGTM.

@bweick bweick merged commit eb8938c into master Apr 26, 2023
7 checks passed
@bweick bweick deleted the brian/apy-rescue branch April 26, 2023 23:51
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.

None yet

2 participants