-
Notifications
You must be signed in to change notification settings - Fork 169
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
Feature/pool 132 implement sponsorship #40
Conversation
…t methods to accept data & operatorData
…ooltogether/pooltogether-pool-contracts into feature/pool-132-implement-sponsorship
import "@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/IERC20.sol"; | ||
|
||
interface CTokenInterface is IERC20 { | ||
function totalSupply() external override view returns (uint256); |
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.
✂️
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.
IERC20 also has a totalSupply() so this has to be "override"
function underlying() external view returns (address); | ||
function balanceOfUnderlying(address owner) external returns (uint256); | ||
function supplyRatePerBlock() external returns (uint256); | ||
function exchangeRateCurrent() external returns (uint256); | ||
function mint(uint256 mintAmount) external returns (uint256); | ||
function balanceOf(address user) external view returns (uint256); | ||
function balanceOf(address user) external override view returns (uint256); |
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.
✂️
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.
IERC20 also has a balanceOf() so this has to be "override"
contracts/token/ControlledToken.sol
Outdated
_burn(_user, _amount, data, operatorData); | ||
} | ||
|
||
// function burn(uint256 amount, bytes memory data) public override { |
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.
✂️
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.
ERC777 has a public "burn" method but it cannot be overridden (not marked virtual)
package.json
Outdated
@@ -11,7 +11,7 @@ | |||
"test": "buidler test", | |||
"compile": "buidler --show-stack-traces --max-memory 8192 compile", | |||
"start": "buidler node --port $LOCAL_BUIDLEREVM_PORT", | |||
"coverage": "rm -rf build cache test-results.xml && buidler compile && buidler coverage --network coverage --temp build; rm -rf cache", | |||
"coverage": "rm -rf build cache test-results.xml && buidler compile && buidler coverage --network coverage --temp build", |
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.
keep the ; rm -rf cache
as the coverage doesn't seem to clean up after itself
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.
Fixed, but I didn't remove this.
}) | ||
}) | ||
|
||
describe('setTokens()', () => { |
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.
Maybe just add a test to ensure this can't be called twice (for the pesky require conditional)
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.
done!
No description provided.