Dynamic tractor data

[pocikerim]

No description provided.

[fr1jo]

Review I. Not bad for the first time 🙂

[fr1jo]

Whenever we have a struct, we should have some NatSpec on what the purpose of this struct is. If the struct is quite complex/the names of the variables don't give enough context on what its used for, then we should also try to comment on the names as well.

here is documentation on natspec:
https://docs.soliditylang.org/en/latest/natspec-format.html

[fr1jo]

@notice is generally used to explain "what" the function does (i.e "Get tractor data"), whereas @dev explains the "how" or "why" the function is implemented this way.

example:

/**
* @notice Fetch tractor data by key.
* @dev this function should be called within Tractor Blueprint Contracts that require dynamically sized data given by Tractor operators.
*/

Its up to the developer to add the @param or @return, but in this case it's pretty obvious on what the values are, so omitting it is fine.

[fr1jo]

1. Generally for all structs, we should always try to add natspec for other developers. Follow the documentation process outlined here: https://docs.soliditylang.org/en/latest/natspec-format.html

2. Structs should be defined after type declarations (a struct itself is a type declaration). You can reference the order in the solidity Style guide: https://docs.soliditylang.org/en/latest/style-guide.html, under Order of Layout. This struct should be moved to Line 25.

[fr1jo]

generally though, we try to adhere to Coinbase's style guide (notably, the exceptions). These are pretty minor though and are only done for readability purposes.

https://github.com/coinbase/solidity-style-guide/blob/main/README.md

[fr1jo]

I would continue to use the same variable naming style in the tractor Function. We should keep the operatorData variable instead of renaming to operatorInjectorData.

Sometimes you will have to make decisions on whether to keep variable naming consistent, or changing it in favor for more detail. Given that solidity changes create downstream effects in both the middleware and frontend, we generally lean on to keeping things as consistent as possible.

Generally with structs, we reference the struct, rather than describing the struct itself.

Below is what i think would be more optimal.

/**
     * @notice Execute a Tractor blueprint with dynamic data injection.
     * @param requisition The blueprint requisition. See {LibTractor.Requisition}
     * @param operatorData Static length data provided by the operator.
     * @param operatorDynamicData generic length data provided by the operator
     * @dev `operatorDynamicData` allows for smart contracts that wrap around Tractor to utilize dynamically sized operator payloads. Tractor operators should default to a key of 0 when injecting data, unless specified within the Tractor smart contract,
* @return results Array of results from executed farm calls
     */
    function tractorDynamicData(
        LibTractor.Requisition calldata requisition,
        bytes memory operatorData,
        ContractData[] memory operatorDynamicData
    )

[fr1jo]

Couple of things:

Key principle in coding is to apply DRY (Do not Repeat) principles whenever possible. This is especially important in solidity as duplicate code causes unnecessary duplication of bytecode, which leads to 1) extra costs when deploying smart contracts, 2) may cause us to hit the code size limit.

Generally, DRY allows for easier maintainability and readability. To give an example, note that the tractor function and the tractorWithData function have 90% shared code with each other. this means that we should have an internal function that is invoked on both tractor and tractorWithData.

function tractor(
        LibTractor.Requisition calldata requisition,
        bytes memory operatorData
    )
        external
        payable
        fundsSafu
        nonReentrantFarm
        verifyRequisition(requisition)
        runBlueprint(requisition)
        returns (bytes[] memory results)
    {
        LibTractor.tractor(...)
   } 



function tractorWithData(
        LibTractor.Requisition calldata requisition,
        bytes memory operatorInjectorData,
        ContractData[] memory contractData
    )
        external
        payable
        fundsSafu
        nonReentrantFarm
        verifyRequisition(requisition)
        runBlueprint(requisition)
        returns (bytes[] memory results)
    {
     LibTractor.setContractData(contractData);
     LibTractor.tractor(...)
     LibTractor.resetContractData(contractData);
     }

You can see how from a bird eyes view (i.e a glance), that

1. the two functions share very similar functionality, and
2. its obvious what the difference is between the two is.

You can argue that we can put the setting and resetting of contract data within a modifier. I think both are fine and is up to the developers preference. personally i find this a bit easier to read as all the context is on the function, and we already have 4 modifiers on this function. Obviously you should comment as much as needed to make this easy to understand.

A side note: when we make upgrade the protocol, something to keep in mind as solidity developers is that we should avoid the function signature changing for existing functions whenever possible, to allow for backwards compatibility. This is why we created a separate tractorWithData function, rather than augmenting the existing tractor function. However, changing the logic within the function is fine.

[fr1jo]

i'd put R342-344 into a internal library function, and have an if statement in the internal function

the rule of thumb is that internal functions are useful once you can explain the block of code in a function name. For example, after we add the if check, this section of code will be 5+ lines of code. its easier for other developers to inuit and black-box logic if the function is in one line.

ex:

// Temporarily store data for external smart contracts to utilize.
LibTractor.setContractData(contractData); 

[fr1jo]

this function should be in LibTractorStorage

also, lets remove the underscore on this name, i strongly prefer that.

[fr1jo]

per our conversation before, setting the value to bytes(1) saves us little value given that the bytes are dynamic here. I believe we use the delete keyword here.

[fr1jo]

lets add natspec here on the struct and on LibTractorStorage. You can look at LibAppStorage for reference.

[fr1jo]

view functions should be below external state changing functions. https://docs.soliditylang.org/en/latest/style-guide.html#order-of-layout