Skip to content

Dynamic tractor data #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pocikerim wants to merge 6 commits into
base: mow-plant-harvest-blueprint
Choose a base branch
from
Open

Dynamic tractor data #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f68ac04
dyanmic data field added to tractorStorage and struct moved to LibTra…
pocikerim Nov 6, 2025
a0f6c43
Replace storage deletion with abi.encode(1) reset for gas optimization.
pocikerim Nov 7, 2025
23afd53
Update TractorFacet.sol
pocikerim Nov 7, 2025
8853051
tests
pocikerim Nov 7, 2025
d8d1b4a
Revert "tests"
pocikerim Nov 7, 2025
b08ba3b
fix: move TractorStorage reference from LibTractor to LibTractorStora…
pocikerim Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions contracts/beanstalk/facets/farm/TractorFacet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ contract TractorFacet is Invariable, ReentrancyGuard {
uint256 gasleft
);

struct ContractData {
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

uint256 key;
bytes value;
}

/**
* @notice Ensure requisition hash matches blueprint data and signer is publisher.
*/
Expand Down Expand Up @@ -298,4 +303,75 @@ contract TractorFacet is Invariable, ReentrancyGuard {
function operator() external view returns (address) {
return LibTractor._getOperator();
}

/**
* @notice Get tractor data by key (for blueprint contract access).
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

* @param key The key to get the data for
* @return The data for the key
*/
function getTractorData(uint256 key) external view returns (bytes memory) {
Copy link

@fr1jo fr1jo Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Image

return LibTractor._getTractorData(key);
}

/**
* @notice Execute a Tractor blueprint with dynamic data injection.
* @param requisition The blueprint requisition containing signature and blueprint data
* @param operatorInjectorData Operator data for fixed-position injection (like normal tractor)
* @param contractData Array of key-value pairs for dynamic data injection
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    )

* @return results Array of results from executed farm calls
*/
function tractorWithData(
LibTractor.Requisition calldata requisition,
bytes memory operatorInjectorData,
ContractData[] memory contractData
)
external
payable
fundsSafu
nonReentrantFarm
verifyRequisition(requisition)
runBlueprint(requisition)
returns (bytes[] memory results)
{
require(requisition.blueprint.data.length > 0, "TractorWithData: data empty");
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


LibTractor._setCurrentBlueprintHash(requisition.blueprintHash);

LibTractor._setOperator(msg.sender);

for (uint256 i = 0; i < contractData.length; i++) {
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

LibTractor._setTractorData(contractData[i].key, contractData[i].value);
}

AdvancedFarmCall[] memory calls = abi.decode(
LibBytes.sliceFrom(requisition.blueprint.data, 4),
(AdvancedFarmCall[])
);

for (uint256 i; i < requisition.blueprint.operatorPasteInstrs.length; ++i) {
bytes32 operatorPasteInstr = requisition.blueprint.operatorPasteInstrs[i];
uint80 pasteCallIndex = operatorPasteInstr.getIndex1();
require(calls.length > pasteCallIndex, "TractorWithData: pasteCallIndex OOB");

LibBytes.pasteBytesTractor(
operatorPasteInstr,
operatorInjectorData,
calls[pasteCallIndex].callData
);
}

results = new bytes[](calls.length);
for (uint256 i = 0; i < calls.length; ++i) {
require(calls[i].callData.length != 0, "TractorWithData: empty AdvancedFarmCall");
results[i] = LibFarm._advancedFarm(calls[i], results);
}

for (uint256 i = 0; i < contractData.length; i++) {
LibTractor._clearTractorData(contractData[i].key);
}

LibTractor._resetCurrentBlueprintHash();

LibTractor._resetOperator();
}
}
3 changes: 2 additions & 1 deletion contracts/beanstalk/init/deployment/InitProtocol.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {BeanstalkERC20} from "contracts/tokens/ERC20/BeanstalkERC20.sol";
import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {AppStorage} from "contracts/beanstalk/storage/AppStorage.sol";
import {LibTractor} from "contracts/libraries/LibTractor.sol";
import {LibTractorStorage} from "contracts/libraries/LibTractorStorage.sol";
import {LibCases} from "contracts/libraries/LibCases.sol";
import {Distribution} from "contracts/beanstalk/facets/sun/abstract/Distribution.sol";
import {C} from "contracts/C.sol";
Expand Down Expand Up @@ -195,7 +196,7 @@ contract InitProtocol {
* @notice Sets the tractor version and active publisher.
*/
function setTractor() internal {
LibTractor.TractorStorage storage ts = LibTractor._tractorStorage();
LibTractorStorage.TractorStorage storage ts = LibTractor._tractorStorage();
ts.activePublisher = payable(address(1));
ts.version = "1.0.0";
}
Expand Down
48 changes: 31 additions & 17 deletions contracts/libraries/LibTractor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

pragma solidity ^0.8.20;

import {LibTractorStorage} from "./LibTractorStorage.sol";

/**
* @title Lib Tractor
**/
Expand All @@ -29,21 +31,6 @@ library LibTractor {

event TractorVersionSet(string version);

struct TractorStorage {
// Number of times the blueprint has been run.
mapping(bytes32 => uint256) blueprintNonce;
// Publisher Address => counter id => counter value.
mapping(address => mapping(bytes32 => uint256)) blueprintCounters;
// Publisher of current operations. Set to address(1) when no active publisher.
address payable activePublisher;
// Version of Tractor. Only Blueprints using current Version can run.
string version;
// Hash of currently executing blueprint
bytes32 currentBlueprintHash;
// Address of the currently executing operator
address operator;
}

// Blueprint stores blueprint related values
struct Blueprint {
address publisher;
Expand All @@ -67,13 +54,40 @@ library LibTractor {
* @notice Get tractor storage from storage.
* @return ts Storage object containing tractor data
*/
function _tractorStorage() internal pure returns (TractorStorage storage ts) {
function _tractorStorage() internal pure returns (LibTractorStorage.TractorStorage storage ts) {
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function should be in LibTractorStorage

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

// keccak256("diamond.storage.tractor") == 0x7efbaaac9214ca1879e26b4df38e29a72561affb741bba775ce66d5bb6a82a07
assembly {
ts.slot := 0x7efbaaac9214ca1879e26b4df38e29a72561affb741bba775ce66d5bb6a82a07
}
}

/**
* @notice Get tractor data by key.
* @param key The key to get the data for
* @return The data for the key
*/
function _getTractorData(uint256 key) internal view returns (bytes memory) {
return _tractorStorage().data[key];
}

/**
* @notice Set tractor data by key.
* @param key The key to set the data for
* @param value The data to set for the key
*/
function _setTractorData(uint256 key, bytes memory value) internal {
_tractorStorage().data[key] = value;
}

/**
* @notice Clear tractor data by key.
* @dev Resets to bytes(abi.encode(1)) instead of zero for gas optimization.
* @param key The key to clear the data for
*/
function _clearTractorData(uint256 key) internal {
_tractorStorage().data[key] = abi.encode(1);
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* @notice Set the tractor hashed version.
*/
Expand Down Expand Up @@ -104,7 +118,7 @@ library LibTractor {
* @param publisher blueprint publisher address
*/
function _setPublisher(address payable publisher) internal {
TractorStorage storage ts = _tractorStorage();
LibTractorStorage.TractorStorage storage ts = _tractorStorage();
require(
uint160(bytes20(address(ts.activePublisher))) <= 1,
"LibTractor: publisher already set"
Expand Down
17 changes: 17 additions & 0 deletions contracts/libraries/LibTractorStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* SPDX-License-Identifier: MIT
**/

pragma solidity ^0.8.20;

library LibTractorStorage {
struct TractorStorage {
Copy link

@fr1jo fr1jo Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mapping(bytes32 => uint256) blueprintNonce;
mapping(address => mapping(bytes32 => uint256)) blueprintCounters;
address payable activePublisher;
string version;
bytes32 currentBlueprintHash;
address operator;
mapping(uint256 key => bytes value) data;
}
}
Loading