programmablewealth / gotchiverse-crowdfarming-contracts Goto Github PK

The transfer call will fail if the owner is not correct, so these checks are redundant. It's fine to keep them if it makes you feel more comfortable, but personally I think it's a wasted check.

This is already a bool, so can be shortened to:

    require(s.farmingOperations[_operationId].landDeposited, "Land must be deposited for this farming operation");

I recommend using the format: "CrowdFundingFacet: Error" as this helps immensely with figuring out from which facets errors come from.

This function only has an external entry point. Because of this, the function requires calling this.calculateBudget. This is dangerous when combined with LibMeta, as LibMeta relies on making external function calls to address(this) to manipulate sender address.

I recommend either:

1. Making the function public so that you don't have to make an external call to your own contract.
2. Making a library that would contain all internal logic. This would be useful if you ever added more facets to your contract, as this internal functionality can be shared between facets.

Since theres a check to make sure that both lengths are equal, only one length needs to be checked > 0

Something like:

uint256 alchemicaCosts = installationTypes[i].alchemicaCosts;
uint256 installationQuantity = _installationQuantities[i];

budget[0] += alchemicaCosts[0] * installationQuantity;
...

It is important to label functions that do not modify state as view, as this helps ensure to contract readers that the function is safe from modifying any state.

These lines should be using internal calls, e.g. just using "calculateBudget()" rather than "this.calculateBudget()"

A public function is both an external and internal function. By using "this", you are telling solidity to call this function externally which changes msg.sender. This also increases gas cost as internal calls are cheaper than external ones.

I recommend defining structs like this:

FarmingOperation(
{
operationId: newOperationId,
landTokenId: _landTokenId,
...
}
)

This increases readability for those unfamiliar with the contracts and reduces the chances of making a mistake with field ordering.