-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix(node): skip power scale if not collateral based #1149
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -4,15 +4,16 @@ | |||
use anyhow::{anyhow, Context}; | ||||
use fendermint_crypto::PublicKey; | ||||
use fvm_shared::address::Address; | ||||
use fvm_shared::econ::TokenAmount; | ||||
use ipc_provider::config::subnet::{EVMSubnet, SubnetConfig}; | ||||
use ipc_provider::IpcProvider; | ||||
use std::path::PathBuf; | ||||
|
||||
use fendermint_vm_actor_interface::eam::EthAddress; | ||||
use fendermint_vm_core::Timestamp; | ||||
use fendermint_vm_genesis::{ | ||||
ipc, Account, Actor, ActorMeta, Collateral, Genesis, Multisig, PermissionMode, SignerAddr, | ||||
Validator, ValidatorKey, | ||||
ipc, Account, Actor, ActorMeta, Collateral, Genesis, Multisig, PermissionMode, PowerScale, | ||||
SignerAddr, Validator, ValidatorKey, | ||||
}; | ||||
use fendermint_vm_interpreter::genesis::{GenesisAppState, GenesisBuilder}; | ||||
|
||||
|
@@ -21,6 +22,8 @@ use crate::options::genesis::*; | |||
|
||||
use super::key::read_public_key; | ||||
|
||||
const DEFAULT_POWER_SCALE: PowerScale = 3; | ||||
|
||||
cmd! { | ||||
GenesisArgs(self) { | ||||
let genesis_file = self.genesis_file.clone(); | ||||
|
@@ -339,6 +342,15 @@ async fn new_genesis_from_parent( | |||
|
||||
let genesis_info = parent_provider.get_genesis_info(&args.subnet_id).await?; | ||||
|
||||
let power_scale = if matches!( | ||||
genesis_info.permission_mode, | ||||
ipc_api::subnet::PermissionMode::Collateral | ||||
) { | ||||
args.power_scale.unwrap_or(DEFAULT_POWER_SCALE) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary? Given the |
||||
} else { | ||||
TokenAmount::DECIMALS as PowerScale | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this be the value we put in set-federated-power? that would be great if so! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cryptoAtwill shouldn't this be 1? We want X federated power to translate directly into X weight, right? Having 18 here would force users to append 18 zeroes to whatever power they actually want to see in the subnet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a subtraction here: ipc/fendermint/vm/genesis/src/lib.rs Line 129 in d881a2e
|
||||
}; | ||||
|
||||
// get gateway genesis | ||||
let ipc_params = ipc::IpcParams { | ||||
gateway: ipc::GatewayParams { | ||||
|
@@ -357,7 +369,7 @@ async fn new_genesis_from_parent( | |||
chain_name: args.subnet_id.to_string(), | ||||
network_version: args.network_version, | ||||
base_fee: args.base_fee.clone(), | ||||
power_scale: args.power_scale, | ||||
power_scale, | ||||
validators: Vec::new(), | ||||
accounts: Vec::new(), | ||||
eam_permission_mode: PermissionMode::Unrestricted, | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,7 +175,7 @@ impl<DB: Blockstore + Clone> GatewayCaller<DB> { | |
.current_membership(state) | ||
.context("failed to get current membership")?; | ||
|
||
let power_table = membership_to_power_table(&membership, state.power_scale()); | ||
let power_table = membership_to_power_table(&membership, state.power_scale())?; | ||
|
||
Ok((membership.configuration_number, power_table)) | ||
} | ||
|
@@ -347,19 +347,57 @@ pub fn tokens_to_burn(msgs: &[checkpointing_facet::IpcEnvelope]) -> TokenAmount | |
fn membership_to_power_table( | ||
m: &gateway_getter_facet::Membership, | ||
power_scale: PowerScale, | ||
) -> Vec<Validator<Power>> { | ||
) -> anyhow::Result<Vec<Validator<Power>>> { | ||
let mut pt = Vec::new(); | ||
|
||
for v in m.validators.iter() { | ||
// Ignoring any metadata that isn't a public key. | ||
if let Ok(pk) = PublicKey::parse_slice(&v.metadata, None) { | ||
let c = from_eth::to_fvm_tokens(&v.weight); | ||
pt.push(Validator { | ||
public_key: ValidatorKey(pk), | ||
power: Collateral(c).into_power(power_scale), | ||
}) | ||
} | ||
let Ok(pk) = PublicKey::parse_slice(&v.metadata, None) else { | ||
return Err(anyhow!( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to fail here? |
||
"metadata not correct public key: {}", | ||
hex::encode(&v.metadata) | ||
)); | ||
}; | ||
|
||
let c = from_eth::to_fvm_tokens(&v.weight); | ||
pt.push(Validator { | ||
public_key: ValidatorKey(pk), | ||
power: Collateral(c).into_power(power_scale), | ||
}) | ||
} | ||
|
||
pt | ||
Ok(pt) | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use crate::fvm::state::ipc::membership_to_power_table; | ||
use fendermint_vm_genesis::PowerScale; | ||
use fvm_shared::econ::TokenAmount; | ||
use ipc_actors_abis::gateway_getter_facet::gateway_getter_facet; | ||
use std::str::FromStr; | ||
|
||
#[test] | ||
fn test_membership_to_power_table() { | ||
let m = gateway_getter_facet::Membership { | ||
configuration_number: 4, | ||
validators: vec![ | ||
gateway_getter_facet::Validator { | ||
weight: ethers::types::U256::from(1), | ||
addr: ethers::types::Address::from_str("0x489Ee71B9E8eDEabB30eBA1b09De783Ee9B85F6B").unwrap(), | ||
metadata: ethers::types::Bytes::from_str("0x0485ff6016b937c0cec4f77cc452d250901aa8ff601faba7f4153a34c94ef9cdd8305e9432be8f94036d243dc1d8d5d0526d656ad179d252cf4dfdb49c78c47c65").unwrap(), | ||
}, | ||
gateway_getter_facet::Validator { | ||
weight: ethers::types::U256::from(1000), | ||
addr: ethers::types::Address::from_str("0x0b18D28e32B2A24e003769960e4F3E262b176399").unwrap(), | ||
metadata: ethers::types::Bytes::from_str("0x0485ff6016b937c0cec4f77cc452d250901aa8ff601faba7f4153a34c94ef9cdd8305e9432be8f94036d243dc1d8d5d0526d656ad179d252cf4dfdb49c78c47c65").unwrap(), | ||
}, | ||
], | ||
}; | ||
|
||
let v = membership_to_power_table(&m, TokenAmount::DECIMALS as PowerScale).unwrap(); | ||
|
||
assert_eq!(v[0].power.0, 1); | ||
assert_eq!(v[1].power.0, 1000); | ||
} | ||
} |
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 think renaming it to something more appropriate (like above) makes sense. Also, we should abstain from using FIL, as the collateral can now include ERC-20 tokens as well.