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

update to support SafeERC20 #2

Merged
merged 2 commits into from
Jul 13, 2021
Merged

update to support SafeERC20 #2

merged 2 commits into from
Jul 13, 2021

Conversation

kamescg
Copy link
Contributor

@kamescg kamescg commented Jun 28, 2021

@@ -3,6 +3,8 @@
pragma solidity 0.6.12;

import { IYieldSource } from "@pooltogether/yield-source-interface/contracts/IYieldSource.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
import "@openzeppelin/contracts/math/SafeMath.sol";
import "./IBadgerSett.sol";
import "./IBadger.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line and the file in the repo.

Comment on lines 22 to 24
constructor(IBadgerSett badgerSettAddr, IERC20 badgerAddr) public {
badgerSett = badgerSettAddr;
badger = badgerAddr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be more readable if it was written this way:

Suggested change
constructor(IBadgerSett badgerSettAddr, IERC20 badgerAddr) public {
badgerSett = badgerSettAddr;
badger = badgerAddr;
constructor(IBadgerSett _badgerSettAddr, IERC20 _badgerAddr) public {
badgerSett = _badgerSettAddr;
badger = _badgerAddr;

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that. It's also consistent with our convention of using _ for function args

@coveralls
Copy link

Pull Request Test Coverage Report for Build 994694853

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.667%

Totals Coverage Status
Change from base Build 994692896: 0.0%
Covered Lines: 28
Relevant Lines: 28

💛 - Coveralls

@PierrickGT PierrickGT changed the base branch from master to fixes/c4-audit July 5, 2021 14:32
@PierrickGT PierrickGT merged commit 328a5d5 into fixes/c4-audit Jul 13, 2021
@PierrickGT PierrickGT deleted the fix/112 branch July 13, 2021 16:12
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.

4 participants