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

Feedback1.2 #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feedback1.2 #3

wants to merge 5 commits into from

Conversation

OpenOrg-gg
Copy link
Owner

No description provided.

@OpenOrg-gg OpenOrg-gg mentioned this pull request Jun 4, 2022
4 tasks
@dudesahn
Copy link

you don't need to set this to address(0), will be that by default

address public tradeFactory = address(0);

@dudesahn
Copy link

for all of the other addresses, you also don't need to re-cast them as address(x)

address public constant strategistMultisig = address(0x16388463d60FFE0661Cf7F1f31a7D658aC790ff7);

just do

address public constant strategistMultisig = 0x16388463d60FFE0661Cf7F1f31a7D658aC790ff7;

@dudesahn
Copy link

you can remove this:

using SafeERC20 for IERC20;

and this:
using SafeMath for uint256;

since these are already inherited from baseStrategy

@dudesahn
Copy link

address public assetMarket = address(want);

This doesn't seem to be used anywhere?

@dudesahn
Copy link

What are the two addresses here? Would be good to at least have a comment:

marketsModule.enterMarket(0, address(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48));

want.approve(address(0x27182842E098f60e3D576794A5bFFb0777E025d3), type(uint).max);

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