refactor: natspec + interface changes #364
Conversation
0xClandestine
force-pushed
the
0xclandestine/natspec
branch
from
f679a15 to
268fc63
Compare
0xClandestine
changed the title
RegistryCoordinator natspec
| import {IBLSApkRegistry} from "./interfaces/IBLSApkRegistry.sol"; | ||
| import {IStakeRegistry, IDelegationManager} from "./interfaces/IStakeRegistry.sol"; | ||
|
|
||
| abstract contract BLSSignatureCheckerStorage is IBLSSignatureChecker { |
There was a problem hiding this comment.
Figured it made sense to separate this out for consistency.
0xClandestine
changed the title
0xClandestine
force-pushed
the
0xclandestine/natspec
branch
from
5a4594b to
8d42bef
Compare
refactor: natspec + interfaces docs: `RegistryCoordinator` natspec chore: forge fmt refactor: named mapping params docs: natspec `BLSApkRegistry` docs: natspec `BLSApkRegistry` docs: natspec `BLSApkRegistry` refactor: interface structure refactor: separate `BLSSignatureChecker` storage refactor: rename -> `IECDSAStakeRegistry` docs: natspec docs: natspec `ECDSAStakeRegistry` docs: natspec `EjectionManager` + separate storage docs: natspec docs: natspec `IndexRegistry` refactor: remove `IRegistry` chore: forge fmt chore: make storage-report docs: natspec refactor: remove `ISocketUpdater`
0xClandestine
force-pushed
the
0xclandestine/natspec
branch
from
3768612 to
7cb4449
Compare
|
|
||
| /** | ||
| * @notice This function is called by disperser when it has aggregated all the signatures of the operators |
There was a problem hiding this comment.
It looks like some of the details here is missing in the natspec in the IBLSSignatureChecker.sol
| @@ -73,13 +52,10 @@ contract BLSApkRegistry is BLSApkRegistryStorage { | |||
|
|
|||
| // Update each quorum's aggregate pubkey | |||
| _processQuorumApkUpdate(quorumNumbers, pubkey.negate()); | |||
| emit OperatorRemovedFromQuorums(operator, getOperatorId(operator), quorumNumbers); | |||
| emit OperatorRemovedFromQuorums(operator, operatorToPubkeyHash[operator], quorumNumbers); | |||
There was a problem hiding this comment.
Would prefer to leave as is using the helper function getOperatorId. Same for registerOperator
There was a problem hiding this comment.
Noticing right now that there isn't a storage gap in the original implementation and would like to add it but
I'm wondering if anyone has deployed and extended an EjectionManager contract because adding the gap now could potentially break some existing AVSs.
| IStakeRegistry | ||
| } from "./EjectionManagerStorage.sol"; | ||
|
|
||
| // TODO: double check order of inheritance since we separated storage from logic... |
There was a problem hiding this comment.
For this TODO: can we compare the forge inspect storage outputs and confirm this?
| interface IEjectionManager is IEjectionManagerErrors, IEjectionManagerEvents { | ||
| /// STATE | ||
|
|
||
| // TODO |
| address ejector | ||
| ) external view returns (bool); | ||
|
|
||
| /// @notice Returns the stake ejection at `index` for quorum `quorumNumber`. |
There was a problem hiding this comment.
For clarity, can we keep the notice as Keeps track of the total stake ejected for a quorum
There was a problem hiding this comment.
nit: Looks like we removed the underscores in the param names. Can we be consistent and remove them for _owner and _quorumEjectionParams as well? The constructor is fine as is.
| event NewPubkeyRegistration( | ||
| address indexed operator, BN254.G1Point pubkeyG1, BN254.G2Point pubkeyG2 | ||
| ); | ||
|
|
||
| // @notice Emitted when a new operator pubkey is registered for a set of quorums | ||
| /// @notice Emitted when `operator`'s pubkey is registered for `quorumNumbers`. |
There was a problem hiding this comment.
Just flagging that we natspect events a bit differently in core (use the /** syntax)
| * 3) `quorumNumbers` is ordered in ascending order | ||
| * 4) the operator is not already registered | ||
| */ | ||
| /// @notice Maps `operator` to their BLS public key hash (`operatorId`). |
There was a problem hiding this comment.
We're also natspec-ing functions different, prefer the old way for consistency
No description provided.