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 StandardToken.sol #224

Closed
wants to merge 1 commit into from
Closed

Conversation

pipaman
Copy link
Contributor

@pipaman pipaman commented May 23, 2017

Added increaseApproval and decreaseApproval to increase / decrease the approval in 1 transaction.

Added increaseApproval and decreaseApproval to increase / decrease the approval in 1 transaction.
@maraoz
Copy link
Contributor

maraoz commented May 23, 2017

I like this approach. Thanks for the contribution
Would you mind adding some tests?

@maraoz
Copy link
Contributor

maraoz commented Jun 22, 2017

@pipaman ping on adding tests ;)

Copy link
Contributor

@rudygodoy rudygodoy left a comment

Choose a reason for hiding this comment

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

I did a little investigation on why the CI build passed when the PR does not include the implementation of the onlyPayloadSize() modifier for the two added functions. Turns out the project's dependencies (EDIT: at the time) got an older version of the solc compiler (0.4.8) which does not complain about such issues as more up to date versions do (0.4.14). Time to upgrade dependencies?

* From MonolithDAO Token.sol
*/
function increaseApproval (address _spender, uint _addedValue)
onlyPayloadSize(2 * 32)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming we want to use the MonolithDAO's implementation of the onlyPayloadSize() modifier? The contract doesn't even build due missing implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind the previous comment about dependencies. I'm working on this.

Copy link
Contributor

@frangio frangio Aug 4, 2017

Choose a reason for hiding this comment

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

onlyPayloadSize was removed (#277). This PR should should have the modifier removed.

rudygodoy added a commit to rudygodoy/zeppelin-solidity that referenced this pull request Aug 20, 2017
Added onlyPayloadSize modifier implementation for PR OpenZeppelin#224.
rudygodoy added a commit to rudygodoy/zeppelin-solidity that referenced this pull request Aug 20, 2017
rudygodoy added a commit to rudygodoy/zeppelin-solidity that referenced this pull request Aug 20, 2017
rudygodoy added a commit to rudygodoy/zeppelin-solidity that referenced this pull request Aug 21, 2017
- Spender starts with 0 tokens allowed to spend
- Spender is granted 50, then decreased it's allowance by 10

Refs PR OpenZeppelin#224
frangio added a commit that referenced this pull request Aug 21, 2017
SylTi pushed a commit to SylTi/zeppelin-solidity that referenced this pull request Aug 24, 2017
- Spender starts with 0 tokens allowed to spend
- Spender is granted 50, then decreased it's allowance by 10

Refs PR OpenZeppelin#224
@theethernaut theethernaut added review and removed review labels Jan 2, 2018
@theethernaut
Copy link
Contributor

@pipaman ping ping

ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
- Spender starts with 0 tokens allowed to spend
- Spender is granted 50, then decreased it's allowance by 10

Refs PR OpenZeppelin#224
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
@facuspagnuolo
Copy link
Contributor

facuspagnuolo commented Mar 22, 2018

Closing since it is already implemented. Thanks!

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

6 participants