Acp 77 updates #585
Acp 77 updates #585
Conversation
cam-schultz
requested review from
iansuvak,
geoff-vball,
bernard-avalabs and
michaelkaplan13
| Specification: | ||
|
|
||
| ``` | ||
| SubnetConversionMessage |
There was a problem hiding this comment.
Doesn't matter at all for this PR, but worth noting that markdown supports tables that provide auto-formatting in the future. It makes it easy to modify in the future without having to manually correct all of the spacing.
|
|
||
| Description: Acknowledges a Validator weight update | ||
| - Signed by: Subnet |
There was a problem hiding this comment.
Not for this PR, but just a heads up that we'll be updating the terminology from subnet -> L1 soon, including for these message names and transaction types.
Subnets will refer to pre-Etna Subnets, where their validator set must also validate the primary network, making it a subset of the primary network validators.
L1s will refer to post-Etna sovereign networks, whose validators sets can be completely independent of the primary network.
| @@ -236,7 +233,7 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida | |||
| if (input.blsPublicKey.length != BLS_PUBLIC_KEY_LENGTH) { | |||
| revert InvalidBLSKeyLength(input.blsPublicKey.length); | |||
| } | |||
| if (input.nodeID == bytes32(0)) { | |||
| if (input.nodeID.length == 0) { | |||
There was a problem hiding this comment.
As of Etna, the P-Chain will only support 20 byte node IDs, but it's planned to add support for other node IDs in the future (particularly 32-byte Ed25519 node IDs).
I've gone back and forth on if we should require 20 byte node IDs here and track them as bytes20, or if we should leave it more open to support those future upgrades by default.
There was a problem hiding this comment.
I elected to use variable length nodeIDs in order to reduce the amount of deviation from the ACP-77 spec. The main consideration to using bytes20 would be potential gas savings, but I have no investigated that.
| blsPublicKey: input.blsPublicKey, | ||
| registrationExpiry: input.registrationExpiry | ||
| remainingBalanceOwner: input.remainingBalanceOwner, |
There was a problem hiding this comment.
Should we validate the PChainOwners here? The requirements are:
- If threshold is 0, addresses must be empty.
- threshold <= len(addresses)
- addresses must be sorted in ascending order
There was a problem hiding this comment.
Good call, added these validation checks
| } | ||
| if (codecID != CODEC_ID) { | ||
| revert InvalidCodecID(); | ||
| // Individual fields are unpacked in their own scopes to avoid stack too deep errors. |
There was a problem hiding this comment.
(Not for this PR). This style of packing/unpacking is a bit error prone. Perhaps it may be worth writing some lex/bison (or some other lexer/parser) scripts to autogenerate these functions so they are easier to validate.
E2e refactor separate suites
Why this should be merged
Updates the Validator Manager contracts to match the updated ACP-77 spec: https://github.com/avalanche-foundation/ACPs/pull/149/files
Note: Until an Avalanchego commit that includes the updated message specifications and corresponding signature handlers is available, e2e tests are not expected to pass.
Review focus:
contracts/validator-managerHow this works
MessageSpec.mdandValidatorMessages.solHow this was tested
CI
How is this documented