Skip to content

Make ERC827 methods payable#838

Merged
AugustoL merged 6 commits into
OpenZeppelin:masterfrom
k06a:feature/payable-erc827
Apr 23, 2018
Merged

Make ERC827 methods payable#838
AugustoL merged 6 commits into
OpenZeppelin:masterfrom
k06a:feature/payable-erc827

Conversation

@k06a

@k06a k06a commented Mar 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

🚀 Description

Make all ERC827 methods payable to pass-through value. I wanna be able to combine several actions in single tx:

  1. Approve MyToken to MySmartContract
  2. Buy MyToken for ETH in Bancor smart contract
  3. MySmartContract to call transferFrom to receive MyTokens

Remarks:

  • Tell me if you are going to merge and I'll add related tests. (added in a897f11)

  • Linter started to give errors. (fixed in 4c28524)

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines

  • ✅ I've added tests where applicable to test my new functionality.

  • 📖 I've made sure that my contracts are well-documented.

  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@k06a k06a mentioned this pull request Mar 26, 2018
Closed
@k06a

k06a commented Mar 26, 2018

Copy link
Copy Markdown
Contributor Author

Can't avoid linter errors with // solhint-disable-next-line avoid-call-value on previous lines

@duaraghav8

duaraghav8 commented Mar 27, 2018

Copy link
Copy Markdown

@k06a This probably doesn't address your primary issue, but if you'd like to avoid linter error then try // solium-disable-next-line security/no-call-value instead of the above

@k06a

k06a commented Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

@duaraghav8 Thanks!

@k06a k06a force-pushed the feature/payable-erc827 branch from 6734d7d to f5ba91d Compare March 29, 2018 06:38
@shrugs shrugs requested a review from AugustoL April 3, 2018 13:21
@AugustoL

AugustoL commented Apr 3, 2018

Copy link
Copy Markdown
Contributor

@k06a I think we are good to merge it. Can you add the tests for it?
Also, instead of disabling solium for the calls maybe you can change solium config in .soliumrc.json ?

@k06a

k06a commented Apr 3, 2018

Copy link
Copy Markdown
Contributor Author

@AugustoL sure, I'll add tests and fix config.

@k06a k06a force-pushed the feature/payable-erc827 branch from f5ba91d to 4c28524 Compare April 12, 2018 07:39
@k06a

k06a commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

@AugustoL added tests

@AugustoL

AugustoL commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

@k06a thanks, it looks fine but do we really need the functions you add it on message helper? I think we dont.

@k06a

k06a commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

@AugustoL are you talking about failOnBuy or buyMessage? We need buyMessage – it is payble.

@AugustoL

AugustoL commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

@k06a ho ok, and cant you use showMessage instead on failOnBuy on the tests?

AugustoL
AugustoL previously approved these changes Apr 13, 2018
View reviewed changes
@k06a

k06a commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

@AugustoL done, also fixed linter warning.

@k06a k06a mentioned this pull request Apr 13, 2018
Closed
AugustoL
AugustoL previously approved these changes Apr 13, 2018
View reviewed changes
@k06a

k06a commented Apr 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@AugustoL any change to get merged?

@AugustoL

AugustoL commented Apr 16, 2018

Copy link
Copy Markdown
Contributor

@k06a Nope, but I dont know if I should merge it with only my approval. @spalladino Wdyt? Ready to go?

@k06a k06a force-pushed the feature/payable-erc827 branch from 3ec1ee5 to 6cf741b Compare April 23, 2018 12:12
@k06a k06a force-pushed the feature/payable-erc827 branch from 8e94cd6 to 92d67ce Compare April 23, 2018 12:54
@k06a

k06a commented Apr 23, 2018

Copy link
Copy Markdown
Contributor Author

@AugustoL just rebased and fixed tests

@AugustoL AugustoL merged commit 8f2a478 into OpenZeppelin:master Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@AugustoL AugustoL AugustoL approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@AugustoL AugustoL

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@k06a @duaraghav8 @AugustoL @shrugs