Suggestions in BaseCrowdsale.sol

[shingonu]

I am auditing this: https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol

I just wonder how you(@4000D) think about these suggestions.

variable name:

• cap -> softCap or hardCap like data field in input json. https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol#L31

unused parameter:

• _beneficiary field not used https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol#L150

logic:

• I would like to divide buyTokens into functions buyTokens and buyTokensAndGenerateToBeneficiary. buyTokens's beneficiary is msg.sender, buyTokensAndGenerateToBeneficiary's beneficiary is address parameter(beneficiary). https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol#L56
• To make an investment explicit, do not put logic in the fallback function, just revert it. https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol#L52
• In my opinion, would not it be better to separate unimplemented functions into different contracts or interfaces? https://github.com/Onther-Tech/tokyo/blob/f7007f39d1868910f0f262163416b4492be9d98b/packages/tokyo-reusable-crowdsale/audit/full-features/contracts/base/crowdsale/BaseCrowdsale.sol#L166

[4000D]

variable name

cap is hard cap of crowdsale. goal is soft cap. these name for Zeppelin's implementation(cap, goal)

unused parameter

calculateToFund function is expected to override like here and here. Even though _beneficiary parameter is not used in BaseCrowdsale contract, other contracts inherit BaseCrowdsale can override calculateToFund function with using _beneficiary parameter.

It is more intuitive to provide variable name for other inheriting contract to override function even it is not used in base contract

logic

1 and 2. We use Zeppelin's Crowdsale contract. buyTokens function you said is fallback function and buyTokensAndGenerateToBeneficiary function you said is buyTokens function in Zeppelin's Crowdsale contract. Transferring ETH to Crowdsale contract itself is absolutely explicit investment process. Nobody send ether to Crowdsale contract in order not to invest.

3. BaseCrowdsale is abstract contract, neither contract or interface. init function is overrided tokyo generated crowdsale contract like here