-
Notifications
You must be signed in to change notification settings - Fork 7
Integrate GasKillerSDK into YieldDistributor #174
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
Conversation
|
Just curios why no CI/CD (not even the simple forge build) ran on this PR - guess the workflow is not configured to target merges into dev I skimmed over the issues and the open PRs and I see #163 which seems to have gone stale, maybe for a good reason |
This repo is set up to require manually triggering of CI/CD for some reason. We should fix that, I agree. For now, this PR won't pass any kind of CI because the first step of building will fail because it now has a dependency on a private repo https://github.com/BreadchainCoop/gas-killer-solidity-sdk |
| function initializeGasKiller(address _avsAddress, address _blsSignatureChecker) public reinitializer(2) { | ||
| _setAvsAddress(_avsAddress); | ||
| _setBlsSignatureChecker(_blsSignatureChecker); | ||
| } |
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.
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 causes a problem with integration into upgradeable contracts that use OZ v5+ because we end up with two different versions of the Initializable contract. We might be able to get it to work by playing around with the remappings in the gas killer repo (i.e. use OZ v5 gas killer contracts, allow OZ v4.9 for eigenlayer-middleware contracts) but it wasn't working out so this was used as a workaround for now.
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 understand...
I still don't like that it's not clear how to initialize GasKillerSDK and it's documented in a github issue that you need to call _setAvsAddress and _setBlsSignatureChecker -
At the very least we need to document this in the source code
Another idea that popped into my head is something like __GasKillerSDK_init_dirty which is like __GasKillerSDK_init without the modifier with a comment above it explaining why we can't have onlyInitializing
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.
Ya I agree, it's not ideal. I can definitely open an issue on the gas-killer-solidity-sdk repo to add documentation on proper usage. I'm not against your suggestion, but want to take another shot at getting it to work properly first. We're definitely cutting some corners here, but the idea is to get the YD updated for this cycle so we can use gas killer for the next distribution.
| function distributeYieldGK() public trackState { | ||
| (uint256 _balance, uint256 _baseSplit, uint256 _votedYield) = _claimAndPrepareYield(); | ||
| (uint256[] memory _currentProjectDistributions, uint256 _totalVotes) = _commitVotedDistribution(); | ||
|
|
||
| _executeAndFinalizeDistribution(_currentProjectDistributions, _totalVotes, _balance, _baseSplit, _votedYield); | ||
| } |
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 the canonical function that GK references, right?
Shouldn't we disable calling it not through gas killer?
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 rollout is just to get the functionality available for next cycle. We want to continue using it like normal for now so we kept both functions. Once we're fully migrated to gas killer with a few successful rounds we can update the contract to remove the old function.
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.
It's not about removing the old function - it's about that the new function is not meant to be called directly
Are we sure that it can't be invoked in a way that messes things up?
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.
It can be called directly if you a user wants, just like the original distributeYield function. There are checks within the function so that it won't distribute if it's not a valid time to distribute. The GK function is just expensive in a scenario where there is a large amount of voters, so we have the gas killer verifyAndUpdate function for that.
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.
@rubydusa it is in fact by design meant to be able to be called directly - this is part of the gas killer value proposition - backwards compatiblity
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.
Pull Request Overview
This PR integrates the GasKillerSDK into the YieldDistributor contract to optimize vote distribution processing and prevent out-of-gas errors. The integration adds a new distributeYieldGK() method that uses off-chain vote aggregation while maintaining backward compatibility with the existing distributeYield() method.
Key Changes:
- Integrated GasKillerSDK for state verification and off-chain vote aggregation
- Refactored vote distribution logic to support both legacy and GasKiller methods
- Added new storage variables to track voters and their distributions
- Updated interfaces to use abstract types (IBread, IERC20Votes) for better modularity
- Added comprehensive test coverage for the GasKiller distribution path
Reviewed Changes
Copilot reviewed 27 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/upgrades/v1.0.5/YieldDistributor.sol | New version snapshot for upgrade validation |
| test/upgrades/v1.0.4/ButteredBread.sol | Pragma and formatting updates for consistency |
| test/upgrades/v1.0.3/ButteredBread.sol | Pragma update for compatibility |
| test/upgrades/v1.0.1/YieldDistributor.sol | Import formatting cleanup |
| test/upgrades/v1.0.0/YieldDistributor.sol | Import formatting cleanup |
| test/YieldDistributor.t.sol | Added GasKiller distribution tests and voting power verification tests |
| test/ButteredBread.t.sol | Reordered imports for consistency |
| src/test/YieldDistributorTestWrapper.sol | Updated comment for clarity on currentVotes meaning |
| src/multipliers/VotingStreakMultiplier.sol | Formatting adjustment |
| src/multipliers/PermanentNFTMultiplier.sol | Added blank line for formatting |
| src/multipliers/NFTMultiplier.sol | Reordered imports |
| src/interfaces/multipliers/IProveableMultiplier.sol | Added blank lines for formatting |
| src/interfaces/multipliers/IOnChainProveableMultiplier.sol | Added blank line for formatting |
| src/interfaces/multipliers/IDynamicNFTMultiplier.sol | Removed extraneous blank line |
| src/interfaces/IYieldDistributor.sol | Added TransferFailed error |
| src/interfaces/IERC20Votes.sol | Added checkpoint access methods |
| src/interfaces/IBread.sol | New interface combining IBreadSource and IERC20Votes |
| src/YieldDistributor.sol | Core integration of GasKillerSDK with vote distribution refactoring |
| src/VotingMultipliers.sol | Updated initialization to support YieldDistributor's new architecture |
| script/upgrades/ValidateUpgrade.s.sol | Updated reference contract version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,5729 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0 ^0.8.20 ^0.8.22; | |||
Copilot
AI
Nov 19, 2025
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.
Multiple pragma statements are specified on a single line. Only one pragma version should be specified. This will cause compilation to fail.
| pragma solidity ^0.8.0 ^0.8.20 ^0.8.22; | |
| pragma solidity ^0.8.0; |
| uint256 _yieldFixedSplitDivisor, | ||
| uint256 _lastClaimedBlockNumber, | ||
| address[] memory _projects | ||
| ) public initializer { |
Copilot
AI
Nov 19, 2025
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 VotingMultipliers.initialize() call has been removed from the initialize function, but the initialization is now handled by initializeVotingMultipliers() in a separate reinitializer. This creates a deployment issue where VotingMultipliers won't be initialized during contract deployment unless initializeVotingMultipliers is called separately, potentially leaving the contract in an incomplete state.
| ) public initializer { | |
| ) public initializer { | |
| __VotingMultipliers_init(); |
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.
@bagelface we will have to re-init the voting mutlipliers?
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.
No, it's already been initialized. This is sort of just a readability clean up so we can better retrace our upgrade history and so I could make the change to the "VotingMultipliers" contract to remove the public initializer function an replace it with the internal __VotingMultipliers_init which is more appropriate for a contract that's designed to be inherited by another contract ("YieldDistributor").
The initializeVotingMultipliers will be locked when we call initializeGasKiller because the reinitializer index for the later function is > than the former.
| * @param newAvsAddress The new AVS address | ||
| * @dev Also updates the namespace for the contract | ||
| */ | ||
| function setAvsAddress(address newAvsAddress) external onlyOwner trackState { |
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 need a method to set the bls signature checker address not the avs address as we dont use that right now
The
gas-killer-solidity-sdkrepo is private so running the CI will fail until we update the workflow with an access token. Figured we didn't want to block progress on this for that reason. You can verify tests pass locally withforge test.Upgrade validation passes if you make some changes to trick the script into disregarding issues with initializers. We messed up the VotingMultipliers deploy, so I think it's a little confused. You can get it to pass if you add a call to
__VotingMultipliers_initin theinitializeGasKillerfunction.