-
Notifications
You must be signed in to change notification settings - Fork 15
Mow, Plant and Harvest Tractor Blueprint #134
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.
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
base: master
Are you sure you want to change the base?
Conversation
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
fr1jo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review I
| beanstalk.getBeanToken(), | ||
| vars.account, | ||
| vars.tipAddress, | ||
| params.opParams.operatorTipAmount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should have 3 (now 4) amounts for tipping, for mowing, planting, harvesting, and optimizing their plot deposits (see the comment related to how we figure out optimized plots).
This is because the gas costs will vary on each user, so we should adjust accordingly. this can be abstracted from the user ofc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Does this mean that we will have to tip for each action after the action execution itself ? (ie we tip for mowing after
shouldMowis true and the mow occurs) . -
Also, how do you envision combining user's plots inside the blueprint? As i understand, sorting of deposits - which is a similar action - should happen periodically by the user and is not called by the SowBlueprint for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, what we want to enable is that a user who signs this blueprint will
- we should summate the tips accordingly and tip once.
- the most ideal scenario is that this blueprint (in addition to mowing, planting, and harvesting), has 2 additional things:
- combines plots once the number of plots are "excessive". we need to determine a way to do this in the best way possible. we can discuss options.
- combines and sorts deposits optimally as well. this one is much harder to enforce on chain, but ideally the user always has sorted deposits if possible. with the convert up bonus, we will have many deposits, which we will need to figure out how to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about putting the optimizations in a separate blueprint that the user signs once and handles both field (plot) optimization and deposit optimization after some configured thresholds?
Added separate tips for mowing planting and harvesting here 63e8113 . Tests need to be updated but stack too deep makes it pretty annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the tests here 57d0be0
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…otocol into mow-plant-harvest-blueprint
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…otocol into mow-plant-harvest-blueprint
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
| } | ||
|
|
||
| // update first plot with combined pods | ||
| s.accts[account].fields[fieldId].plots[plotIndexes[0]] = totalPods; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to update plotIndexes and piIndexes as well. we use PodTransfer.insertPlot(...) here. Ideally, we should update the fieldFacet to use insertPlot in LibDibbler as i was investigating and it unfortunately wasn't implmented .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually do not need to do that. By setting the amount of the first plot to be the total merged amount, we preserve the correct piIndex and we dont need to pop the plot from the indexes array. This is like resizing the first plot. Added some assertions for this in the tests and updated the field facet to use the new LibDibbler function here f2fd59f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we preserve the correct piIndex
I don't think thats correct - say that the user has
[ 4000, 5000, 3000]
and we combine [3000, 4000, 5000]
wouldn't the PiIndex change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can you move the tests related to combining plots to the field tests? why is it in the mow plant harvest blueprint?
- can you have a test that attempts to combine a plot like the example above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we preserve the correct piIndex
I don't think thats correct - say that the user has
[ 4000, 5000, 3000]
and we combine [3000, 4000, 5000]wouldn't the PiIndex change?
From Account.sol piIndex: A mapping from Plot index to the index in plotIndexes. . The plot index in the plot indexes array for the first plot does not change. Thus, from my understanding the piIndex that points to the same index in the array does not need an update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can you move the tests related to combining plots to the field tests? why is it in the mow plant harvest blueprint?
- can you have a test that attempts to combine a plot like the example above?
Im in the process of refactoring and cleaning up the tests, including this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up tests here: 57d0be0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tests, there isn't a test where we check that the final plot index that's being merged into is not the one at the earliest index. We need a test that tries to combine the plots in the example above. Even if we don't think that this behavior would break from our understanding, it is good and standard practice to test these edge cases.
you can do this by doing multiple sets of sows like you do in setUpMultipleConsecutiveAccountPlots, then send the earliest plot to another account and back. this will put the earliest index at the end.
ex: given someones index of [1000, 2000, 3000] -> send plot index 1000 to someone else -> index is now [2000, 3000], user receives index 1000 -> index is now [2000, 3000, 1000].
The most ideal test would be one where we have an account whose plotIndexes are very messed up. Example (assume each plot is 1k) [5000, 3000, 6000, 4000, 5000, 0, 2000, 1000]. after the sort, would be great if we also look at the piIndex mapping and check that all the plots (except 0) is the uint256max.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test here by creating a mock function that updates the piIndexes array in storage and performs all the checks. 7843312
|
|
||
| // delete subsequent plot, plotIndex and piIndex mapping entry | ||
| delete s.accts[account].fields[fieldId].plots[plotIndexes[i]]; | ||
| LibDibbler.removePlotIndexFromAccount(account, fieldId, plotIndexes[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking on this more, this may not be the most optimal function. removePlotIndexFromAccount updates plots,plotIndexes, and piIndex. The issue is when one of the plots in plotIndexes is a plot at the end of the users plotIndexes.
example:
if plots [3000,4000,5000] are being merged, and the users plotIndexes is [100, 200, 3000, 5000, 4000]:
plotIndexes after first loop:
[100, 200, 4000, 5000]
plotIndexes after 2nd loop:
[100, 200, 5000]
plotIndexes after 3rd loop:
[100, 200]
in here, piIndex(5000) was set 3 times unnecessarily.
what we really want is a function that removes the plot, but keeps the "hole" in the users plot indexes. The only cases where it doesn't is if the plot is the last plot in the array (in that case, we should pop).
after that, updates the users plot indexes accordingly (removing the holes in the most efficient way). at the end, we then add the final new plot.
there could be value in updating the beanstalk functions removePlotIndexFromAccount to account for the state where field.plotIndexes[field.plotIndexes.length - 1] == field.plotIndexes[i], and reverting, but does seem marginal only if we have to update the Field Facet (which we now are to add this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If plots [3000,4000,5000] are being merged the function never removes the first plot but instead caches it and then resizes it, preserving the piIndex and plotIndexes array so the final result would be [100, 200, 3000] with plot[3000] having the total amount of pods. If we don't perform the "swap and pop" that removePlotIndexFromAccount does while merging and we defer everything at the end, we would firstly need to do this operation and also update the piIndex for each plotIndex removed by setting it to type(uint256).max in order to not have dirty storage. So what you are describing saves this operation from happening field.piIndex[field.plotIndexes[i]] = i; n - 1 times where n is the number of total plot indexes to merge. For simplicity, we could add a flag to the function (or write a separate function) thats skip this step or deletes the piIndex mapping entry for that index entirely since we know we wont be using it later. This also provides us with some gas refunds per piIndex entry cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But generally this seems to introduce complexity which i'm not sure is worth having if the gas cost increases by ~n*5000 for 1 transaction periodically ( 0,000000000007369547 (eth per gas) * 100 (plots) * 5000 ( gas of sstore from non zero to non zero value) * 4000 (current eth price) ~= 0,014 cents once every 100 plots for example ( a total of 1,24 usd annualized if this is the only account that sows every hour. )
| // Plant | ||
| uint256 minPlantAmount; | ||
| // Harvest | ||
| uint256 minHarvestAmount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we update this to instead make this a minHarvestAmount on a per field basis? minimal change to support all field ids (i.e someone can use this for unripe pods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to having something like an array of configs for each fieldId in the params instead ? After an initial draft, this turns out to not be super minimal since we would need to loop to get all harvestable plot info per field id and do a refactoring in general.
/**
* @notice Struct to hold field-specific harvest configuration
* @param fieldId The field ID to harvest from
* @param minHarvestAmount The minimum harvestable pods threshold for this field
*/
struct FieldHarvestConfig {
uint256 fieldId;
uint256 minHarvestAmount;
}
/**
* @notice Struct to hold mow, plant and harvest parameters
* @param minMowAmount The minimum total claimable stalk threshold to mow
* @param mintwaDeltaB The minimum twaDeltaB to mow if the protocol
* is close to starting the next season above the value target
* @param minPlantAmount The earned beans threshold to plant
* @param fieldHarvestConfigs Array of field-specific harvest configurations
* -----------------------------------------------------------
* @param sourceTokenIndices Indices of source tokens to withdraw from
* @param maxGrownStalkPerBdv Maximum grown stalk per BDV allowed
* @param slippageRatio The price slippage ratio for lp token withdrawal.
* Only applicable for lp token withdrawals.
*/
struct MowPlantHarvestParams {
// Mow
uint256 minMowAmount;
uint256 mintwaDeltaB;
// Plant
uint256 minPlantAmount;
// Harvest, per field id
@> FieldHarvestConfig[] fieldHarvestConfigs;
// Withdrawal plan parameters for tipping
uint8[] sourceTokenIndices;
uint256 maxGrownStalkPerBdv;
uint256 slippageRatio;
}
....There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tested and ready for review. Open to comments on optimizations #151
|
|
||
| // update first plot with combined pods | ||
| s.accts[account].fields[fieldId].plots[plotIndexes[0]] = totalPods; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have a new event here that combines plots, given new functionality. cc @PintoPirate for the best signature but i assume the array of plots and their amounts would be sufficient (perhaps the array of plots, the total final amount, and the new plot is enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimally the subgraphs will need:
- fieldId
- account
- list of plot indices
I don't think the amounts are particularly helpful but feel free to include. Maybe the new total final amount would be useful if we want some aggregate stat on how many plots were combined in this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an event here with the requested fields. dd1f36f. @PintoPirate Lmk if you need anything else
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…otocol into mow-plant-harvest-blueprint
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…otocol into mow-plant-harvest-blueprint
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…-org/protocol into draft-per-field-id-tractor-harvest
…vest Per-FieldId Tractor Harvest
Summary
Introduce the ability for a user to sign a blueprint that enables automated mowing planting and harvesting under certain conditions.
Key Features
Smart Mow System
twaDeltaB(10e6) and claimable stalk (1 stalk) thresholdsPlant & Harvest Automation
Operator Management
Current Parameters
Implementation Details
The blueprint enables yield optimization while maintaining user control through configurable parameters.