added totalStake and totalValidatorStake along with unit tests #3624
Conversation
There was a problem hiding this comment.
This looks like an accidental submodule downgrade. Probably missing a git submodule update after pulling main.
contracts/src/StakeTableV2.sol
Outdated
| uint16 public maxCommissionIncrease; | ||
|
|
||
| /// @notice Total stake in active (not marked for exit) validators in the contract | ||
| uint256 public totalValidatorStake; |
There was a problem hiding this comment.
I'm not sure about the naming because I couldn't tell just from the names what the variables track.
I feel like it would be best to have pendingWithdrawals, pendingExits, activeStake and totalTracked but this would be 4 storage edits for each delegation which is quite expensive.
There was a problem hiding this comment.
the delegate gas cost is up 46% so i'm looking at other gas saving measures
There was a problem hiding this comment.
Did you make sure this isn't the first delegation? The first one will create 4 new storage entries, subsequent ones only edit the 4 entries.
There was a problem hiding this comment.
But it also sounds about right/expected. A delegation doesn't do much other than moving the token (changing balance of delegator and stake table) and increasing the balance of the delegator for the validator in the stake table.
Overall it feels not ideal to have a user pay so much for something that isn't useful for them. However at current gas prices it's probably fine.
There was a problem hiding this comment.
that's fair at first delegation, in this case, i used the average fees to compare.
If we only store totalValidatorStake which i'm thinking to rename to totalEffectiveStake or totalConsensusStake, the (average) gas increase for a delegation is 25%.
I've made the change to only track totalValidatorStake here, if you agree, i'll merge it into this pr #3630
* only track totalValidatorStake * Return solidity version of proof for reward account (#3611) * change reward proof endpoint to return RewardAccountQUeryData * rename RewardMerklizedStateDataSource -> RewardAccountProofDataSource * fix test * sol reward proof * ... * test sol proof * add api docs * add insta snapshot * ignore snapshots in typos.toml * change insta snapshot dir * fix bindings * revert downgrade of bn254 submodule * Return all reward claim inputs from reward API (#3621) * Squashed commit of the following: commit ad363ab Merge: d1f6ded 7dee60e Author: Mathis <[email protected]> Date: Tue Sep 23 13:36:41 2025 +0200 Merge branch 'main' into ma/reward-claim-contract-interface commit d1f6ded Author: sveitser <[email protected]> Date: Thu Sep 18 10:50:04 2025 +0200 add IRewardClaim ABI Json commit 39ee5de Author: sveitser <[email protected]> Date: Wed Sep 17 20:12:40 2025 +0200 rename event for consistency commit a639520 Author: sveitser <[email protected]> Date: Wed Sep 17 17:01:25 2025 +0200 add IRewardClaim interface This is taken from PR #3491 Would like to review and merge it into main first before we finish the contract so that it's easy to share with external parties. * reward API: return reward claim input - Use jellyfish compat - Use IRewardClaim interface * fix typo * rename API, distinguish internal/external error * improve docs, error logging * WIP: use bytes for authData (cherry picked from commit 87ca2a1) * reward claim: use bytes for auth root data - Add rust types to clarify serialization for auth data. - Add tests for abi encoding and decoding - Update solidity tests to use the new interface. * auth data: remove custom serde impl --------- Co-authored-by: Mathis Antony <[email protected]> * field rename to activeStake --------- Co-authored-by: Abdul Basit <[email protected]> Co-authored-by: Mathis Antony <[email protected]>
|
@alysiahuggins this needs to be synced with main |
Closes #<ISSUE_NUMBER>
This PR:
totalStakeandtotalValidatorStakeThis PR does not:
Key places to review:
How to test this PR: