-
Notifications
You must be signed in to change notification settings - Fork 16
mow plant harvest blueprint with dynamic tractor data #168
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: frijo/release/PI-wsteth-referral
Are you sure you want to change the base?
mow plant harvest blueprint with dynamic tractor data #168
Conversation
🤖 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>
🤖 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>
|
SiloHelpers uses |
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…oHelpers instead of redefining them locally
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…o-org/protocol into pocikerim/mow-plant-harvest
🤖 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 II
…d operator validation and tip resolution logic
…olvedSourceIsBean and _enforceWithdrawalPlanAndTip
🤖 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 III. needs 1 more small-medium refactor, but 95% of the way there!
| * @param fieldId The field ID this data is for | ||
| * @param harvestablePlotIndexes Array of harvestable plot indexes | ||
| */ | ||
| struct OperatorHarvestData { |
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.
the OperatorHarvestData and UserFieldHarvestResults struct has similar data types. any chance this can be combined and used as one?
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.
not applicable anymore from another comment. UserFieldHarvestResults will now return an additional parameter.
| if (vars.userFieldHarvestResults[i].harvestablePlots.length == 0) continue; | ||
|
|
||
| // Harvest the pods to the user's internal balance | ||
| uint256 harvestedBeans = beanstalk.harvest( |
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.
you can save an instantiation here by setting the output of beanstalk.harvest() to vars.totalHarvestedBeans
| } | ||
|
|
||
| // Decode operator-provided harvest data | ||
| OperatorHarvestData memory harvestData = abi.decode( |
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 incorrectly thought this was resolved. given that the tractor operator is already setting the operator data at the harvest key + field id, asking the operator to provide the field id is redundant.
lets rework this so that the operator only needs to set the plots in here. you can remove the require statement from R374 once you do so.
|
|
||
| // If operator didn't provide data for this field, treat as no harvestable pods | ||
| if (operatorData.length == 0) { | ||
| userFieldHarvestResults[i] = UserFieldHarvestResults({ |
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.
one last optimization. Currently, what we do in this function is set the harvestable plots for a given fieldId to 0, If the operator didn't provide the plots (i.e there are no harvestable plots). userFieldHarvestResults is then propagated up to _getAndValidateUserState and vars.userFieldHarvestResults.
lets do the following refactors.
- instantiate a counter, signifying the final length of
userFieldHarvestResults. ifoperatorData.length == 0for a given fieldId, decrement this counter. Once you finish, you can reduce the length ofuserFieldHarvestResultsusing assembly.
something like this:
userFieldHarvestResults = new UserFieldHarvestResults[](fieldHarvestConfigs.length);
uint256 index;
for (uint256 i = 0; i < fieldHarvestConfigs.length; i++) {
uint256 fieldId = fieldHarvestConfigs[i].fieldId;
// Read operator-provided data from transient storage
bytes memory operatorData = beanstalk.getTractorData(HARVEST_DATA_KEY + fieldId);
// If operator didn't provide data for this field, treat as no harvestable pods
if (operatorData.length == 0) {
continue;
}
// Decode operator-provided harvest data
OperatorHarvestData memory harvestData = abi.decode(
operatorData,
(OperatorHarvestData)
);
// Verify operator provided data for the correct field
require(harvestData.fieldId == fieldId, "MowPlantHarvestBlueprint: Field ID mismatch");
// Use operator data - validation happens in harvest() call
userFieldHarvestResults[index] = UserFieldHarvestResults({
fieldId: fieldId,
harvestablePlots: harvestData.harvestablePlotIndexes
});
index++;
}
assembly {
mstore(userFieldHarvestResults, index)
}
-
update
_getAndValidateUserStateto omitshouldHarvest. this is redundant as all the information we need is inuserFieldHarvestResultsin order to know whether we should harvest (ifuserFieldHarvestResults.length != 0). you can also removeshouldHarvestfrom the localVars now as well as removeshouldHarvest = _checkHarvestConditions(userFieldHarvestResults); -
in the
if (vars.shouldHarvest) {block, you can now omit theif (vars.userFieldHarvestResults[i].harvestablePlots.length == 0) continue;check. Given that theuserFieldHarvestResultsindex doesn't match withfieldHarvestConfigsnow, I would update theUserFieldHarvestResultsstruct to include the minHarvestAmount as well. You'll need to slightly refactorrequire( harvestedBeans >= params.mowPlantHarvestParams.fieldHarvestConfigs[i].minHarvestAmount, "MowPlantHarvestBlueprint: Harvested amount below minimum threshold" );to use the value inuserFieldHarvestResults.
| int96 plantedStem, | ||
| uint256 maxGrownStalkPerBdv, | ||
| uint256 slippageRatio | ||
| ) internal { |
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.
you need to add back the previous logic where we only perform this special logic if the token they want to tip from is the pinto token. Unclear why that was removed. should be able to copy paste this directly from a previous commit.
905d012#diff-00263ed26b5d1f84db9835491aa9430aa4d226b0919134fd66fef150d7ad8c1aL594-L624
also, in. _resolvedSourceIsBean, if (sourceTokenIndices.length == 0) return false; is redundant. _validateSourceTokens already checks that sourceTokenIndices.length > 0.
| struct MowPlantHarvestParams { | ||
| // Mow | ||
| uint256 minMowAmount; | ||
| uint256 mintwaDeltaB; |
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.
twa needs to be capitalized here (minTwaDeltaB)
No description provided.