-
Notifications
You must be signed in to change notification settings - Fork 125
Use StakeTableV2 and new events in the GCL #3289
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
I mainly want to test if the asana linking from github works https://app.asana.com/1/1208976916964769/project/1210098463466349/task/1210174253354698
Using current jellyfish APIs it's not possible to construct the type currently used. Since the contract doesn't need it we can send a bytes type instead.
7a67542
to
c789fc2
Compare
This isn't complete yet, but it compiles.
types/src/v0/impls/stake_table.rs
Outdated
|
||
let stake_table_key: BLSPubKey = blsVK.clone().into(); | ||
let state_ver_key: SchnorrPubKey = schnorrVK.clone().into(); | ||
// TODO(MA): The stake table contract currently enforces that each bls key is only used once. We will |
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.
Need to update this comment
impl Serialize for ValidatorRegisteredV2 { | ||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> |
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.
why are we adding on this? so .abi_encode()
is not sufficient?
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 store the events locally in the DB for that I think they are wrapped into an enum.
espresso-network/types/src/v0/v0_3/stake_table.rs
Lines 96 to 105 in 7617c22
#[derive(Clone, derive_more::From, PartialEq, serde::Serialize, serde::Deserialize)] | |
pub enum StakeTableEvent { | |
Register(ValidatorRegistered), | |
RegisterV2(ValidatorRegisteredV2), | |
Deregister(ValidatorExit), | |
Delegate(Delegated), | |
Undelegate(Undelegated), | |
KeyUpdate(ConsensusKeysUpdated), | |
KeyUpdateV2(ConsensusKeysUpdatedV2), | |
} |
It would probably also work with abi_encode / decode but to change now we could have to migrate storage of any nodes that have this deployed already. We would still have to implement serde I think to work with our storage code.
Correct me if I'm wrong @imabdulbasit .
Foundry has a workaround for the lack of function overloading in rust that leads to confusing function names in the bindings. Avoiding the function name collison makes the workaround unnecessary.
- Add tests with V2 register and key update events. - Fix bug where update keys does not check for key-reuse, with regression test. - Make types a bit more convenient with From and Copy impls.
if bls_keys.contains(&stake_table_key) { | ||
bail!("bls key {} already used", stake_table_key.to_string()); | ||
bail!("bls key already used: {}", stake_table_key.to_string()); |
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 diff is a bit misleading here. For the V1 there shouldn't be any changes other than doing the formatting for errors differently and removing some clones because some types are Copy now. I guess since the two branches for v1 and v2 are so similar the diff got cut in half.
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.
lgtm with a few clarifying qs
BN254.G2Point blsVk, | ||
EdOnBN254.EdOnBN254Point schnorrVk, | ||
BN254.G2Point blsVK, | ||
EdOnBN254.EdOnBN254Point schnorrVK, | ||
uint16 commission, | ||
BN254.G1Point blsSig, |
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.
since we made schnorrSig bytes, any chance it makes sense to do so for blsSig too? cc @alxiong
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 actually don't like to use Bytes (anymore) if we have a type of known size because it means we can't make the type Copy in rust.
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.
But IIRC we are missing some code so if we don't use bytes things would have gotten more complicated.
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.
personally I like bytes
because it's more future proof. but whenever we want to do something with these objects in the contract, we should consider giving them a concrete types.
In this case, we do verify the BLS signature, thus giving it a type is better IMO.
I hear Mathis's complain about no Copy
for bytes, but my personal tolerance for Clone
is higher and I can live with lots of .clone()
everywhere, (clippy will inform us about unnecessary clone once alloy bindings decide to add Copy on them)
impl Copy for crate::sol_types::G1PointSol {} | ||
impl Copy for crate::sol_types::G2PointSol {} | ||
impl Copy for crate::sol_types::EdOnBN254PointSol {} | ||
impl Copy for crate::sol_types::StakeTableV2::ValidatorRegistered {} |
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 totally sure but would this also need to be called ValidatorRegisteredV2?
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 this is the the V1 event defined in the V2 abi. The rust code is now written to always use the V2 ABI/bindings (except for when deploying the original StakeTable contract) because the V2 ABI it's a superset of the V1 ABI. I think I left some comments about this in the stake table contract.
sigma: sigma_affine.into_group(), | ||
} | ||
}; | ||
if !bls_vk.validate(&sig, &msg) { |
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 might have missed it but do we have a test for bls vk validation against its sigs?
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.
contracts/rust/deployer/Cargo.toml
Outdated
@@ -14,6 +14,7 @@ derive_more = { workspace = true } | |||
espresso-types = { path = "../../../types" } | |||
hotshot-contract-adapter = { workspace = true } | |||
hotshot-types = { workspace = true } | |||
sequencer-utils = { version = "0.1.0", path = "../../../utils" } |
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.
Only test-utils? So dev-dependency?
staking-cli/tests/cli.rs
Outdated
#[case::v1(StakeTableContractVersion::V1)] | ||
#[case::v2(StakeTableContractVersion::V1)] |
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.
Are they supposed to be the same?
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.
uff, no, thanks
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.
I haven't found a way to sort of "inherit" the test name from the struct. The test function name is more explicit if #[values]
are used such as here:
espresso-network/staking-cli/tests/cli.rs
Lines 361 to 372 in ebe7026
#[rstest::rstest] | |
#[tokio::test] | |
async fn stake_for_demo_delegation_config_helper( | |
#[values(StakeTableContractVersion::V1, StakeTableContractVersion::V2)] | |
version: StakeTableContractVersion, | |
#[values( | |
DelegationConfig::EqualAmounts, | |
DelegationConfig::VariableAmounts, | |
DelegationConfig::MultipleDelegators | |
)] | |
config: DelegationConfig, | |
) -> Result<()> { |
test stake_for_demo_delegation_config_helper::version_2_StakeTableContractVersion__V2::config_1_DelegationConfig__EqualAmounts ... ok
test stake_for_demo_delegation_config_helper::version_2_StakeTableContractVersion__V2::config_2_DelegationConfig__VariableAmounts ... ok
test stake_for_demo_delegation_config_helper::version_1_StakeTableContractVersion__V1::config_1_DelegationConfig__EqualAmounts ... ok
test stake_for_demo_delegation_config_helper::version_1_StakeTableContractVersion__V1::config_3_DelegationConfig__MultipleDelegators ... ok
test stake_for_demo_delegation_config_helper::version_2_StakeTableContractVersion__V2::config_3_DelegationConfig__MultipleDelegators ... ok
But for the applied template it just uses case_1
and case_2
by default.
What's not done
There's currently no E2E test where we start on the original stake table and move to V2 and generate all the events for that. I think we can consider adding it but we are missing good testing infra for doing all of this, so I think it's better to work on this as a separate PR since this one is already much bigger than anticipated.