Update ERC827.sol to not use function overloading

[elie222]

🚀 Description

Renaming of ERC827 function names to avoid using function overloading. This also needs to be accepted by Ethereum team.

This causes problems with projects such as Vyper:
vyperlang/vyper#738

• 📘 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).

[AugustoL]

@elie222 can you make the changes in the tests to run with the new method names?

[elie222]

Yes. I will do this

…

On Thu, 5 Apr 2018, 03:56 Augusto Lemble, ***@***.***> wrote: @elie222 <https://github.com/elie222> can you make the changes in the tests to run with the new method names? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8oX5MLiF8Am6vFGnW-YcCnuk-4pTngks5tlXmjgaJpZM4THrfL> .

[elie222]

I updated the tests and they're all passing now.

[AugustoL]

@elie222 They are not passing in travis, https://travis-ci.org/OpenZeppelin/zeppelin-solidity/jobs/362430479#L3538

[elie222]

Yup. Sorry. Missed running some tests locally. Should work on Travis now

[AugustoL]

@elie222 great, with the new functions names the findMethod function in ERC827Token tests wont be needed, can you remove it too, it will make tests more readable?

[elie222]

Yes makes sense. Will try and get around to it tomorrow.

…

On Thu, 5 Apr 2018, 04:42 Augusto Lemble, ***@***.***> wrote: @elie222 <https://github.com/elie222> great, with the new functions names the findMethod function in ERC827Token tests wont be needed, can you remove it too, it will make tests more readable? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8oXx8yifybun5hHPNlM4pTkmtEyM4Aks5tlYSMgaJpZM4THrfL> .

[k06a]

Maybe to keep both method names?

[elie222]

If we're against this because it's considered "dangerous", then would be best not to? It's early enough in the life of erc827 that we can still remove the previous names?

…

On Thu, 5 Apr 2018, 09:32 Anton Bukov, ***@***.***> wrote: Maybe to keep both method names? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8oX_LVoZZ7Ib8bfRNOfocCpoXPe7mrks5tlcingaJpZM4THrfL> .

[AugustoL]

is early enough, we dont want want duplicated code, I think we should keep the new names that avoid overloading.

[k06a]

@AugustoL no need to duplicate code, approve(address,uint256,bytes) can just call approveAndCall(address,uint256,bytes)

[elie222]

Removed findMethod. Tests and linter passing

[spalladino]

The changes LGTM. @AugustoL I'd say we merge this as soon as this is updated in the EIP, what do you think? Or would you rather merge this right away?

FWIW, I'm not a fan of avoiding function overloading just because it's part of the philosophy of one of the high-level languages available for the EVM. But if the ERC is changing, I think it's fair to change it here as well.

[AugustoL]

@elie222 can you fix the conflicts in ERC827.sol so I can merge it?

[elie222]

Done