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
Refactor *Data methods in LifToken to use super methods #280
Conversation
contracts/LifToken.sol
Outdated
} | ||
|
||
super.approve(spender, value); | ||
assert(spender.call(data)); |
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.
I think here we should use require instead of assert.
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.
Agreed, I just changed it
Changes Unknown when pulling 8a590db on reuse-transfer-in-transfer-data into ** on master**. |
contracts/LifToken.sol
Outdated
} | ||
|
||
super.transferFrom(from, to, value); | ||
require(to.call(data)); |
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.
I think it should be first the require to save some gas, because if it fails later the gas used by transfer will be consumed.
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.
Agreed, just changed it in d818a37
The end result is the same. But this way it will be more natural to replace the first part of the methods with calls to the underlying methods transfer, transferFrom and approve
Calls super.approve, super.transfer & super.transferFrom from the corresponding *Data methods (approveData, transferData & transferDataFrom)
First do the contract call, then the approve/transfer. This way some gas is saved in case the call fails because the approve/transfer is not executed
d818a37
to
b508102
Compare
Calls super.approve, super.transfer & super.transferFrom from the corresponding
*Data methods (approveData, transferData & transferDataFrom)