refactor HasNoTokens.sol to extract reclaimToken

[SylTi]

By splitting reclaimToken of HasNoTokens.sol, it can be used on a contract that can receive a specific type of token, or receive them only a certain way (through transferFrom for example) but want to be able to reclaim all others. inheriting from a HasNoTokens on that type of contract seems counter-intuitive?

[maraoz]

Love this, but I'm not 100% convinced with the name CanReclaimToken. @frangio if you're ok with it, please merge. If not, can you think of a better name? I coudn't

[frangio]

Great idea!

I agree the name is not ideal, but we can keep it if we have nothing better.

[frangio]

You can avoid this cast by defining the function to take an ERC20Basic instance: reclaimToken(ERC20Basic token). (I also think token is a better variable name than tokenInst.)

[frangio]

Please use a function expression instead of an arrow function here. (See https://mochajs.org/#arrow-functions)

[SylTi]

The cast was already there. Didn't touch the code. This is not used in the test. Le mer. 9 août 2017 18:40, Francisco Giordano <notifications@github.com> a écrit :

…

***@***.**** requested changes on this pull request. Great idea! I agree the name is not ideal, but we can keep it if we have nothing better. ------------------------------ In contracts/ownership/CanReclaimToken.sol <#348 (comment)> : > +import "../token/ERC20Basic.sol"; + +/** + * @title Contracts that should be able to recover tokens + * @author SylTi + * @dev This allow a contract to recover any ERC20 token received in a contract by transfering the balance to the contract owner. + * This will prevent any accidental loss of tokens. + */ +contract CanReclaimToken is Ownable { + + /** + * @dev Reclaim all ERC20Basic compatible tokens + * @param tokenAddr address The address of the token contract + */ + function reclaimToken(address tokenAddr) external onlyOwner { + ERC20Basic tokenInst = ERC20Basic(tokenAddr); You can avoid this cast by defining the function to take an ERC20Basic instance: reclaimToken(ERC20Basic token). (I also think token is a better variable name than tokenInst.) ------------------------------ In test/CanReclaimToken.js <#348 (comment)> : > @@ -0,0 +1,35 @@ +'use strict'; +import expectThrow from './helpers/expectThrow'; +import toPromise from './helpers/toPromise'; +const CanReclaimToken = artifacts.require('../contracts/ownership/CanReclaimToken.sol'); +const BasicTokenMock = artifacts.require("./helpers/BasicTokenMock.sol"); + +contract('CanReclaimToken', function(accounts) { + let token = null; + let canReclaimToken = null; + + beforeEach(async () => { Please use a function expression instead of an arrow function here. (See https://mochajs.org/#arrow-functions) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#348 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABsFfNHCepZiUg_kk1RX0ESgCFGmcocCks5sWeD9gaJpZM4OsULr> .

[frangio]

Merging this but it may be superceded by the more general TokenWallet of #378.