Skip to content
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

Flatten instantiation config / params #103

Open
Tracked by #84
maurolacy opened this issue Jan 16, 2025 · 1 comment
Open
Tracked by #84

Flatten instantiation config / params #103

maurolacy opened this issue Jan 16, 2025 · 1 comment

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Jan 16, 2025

We're using optional config / params fields for contract instantiation (

pub params: Option<Params>,
) and providing default values through derivative (in case the entire config / params field is not set) (
#[cw_serde]
#[derive(Derivative)]
#[derivative(Default)]
pub struct Params {
/// `covenant_pks` is the list of public keys held by the covenant committee each PK
/// follows encoding in BIP-340 spec on Bitcoin
pub covenant_pks: Vec<String>,
/// `covenant_quorum` is the minimum number of signatures needed for the covenant multi-signature
pub covenant_quorum: u32,
/// `btc_network` is the network the BTC staking protocol is running on
#[derivative(Default(value = "Network::Regtest"))]
pub btc_network: Network,
// `min_commission_rate` is the chain-wide minimum commission rate that a finality provider
// can charge their delegators
// pub min_commission_rate: Decimal,
/// `slashing_pk_script` is the pk script that the slashed BTC goes to.
/// The pk script is in string format on Bitcoin.
#[derivative(Default(
value = "String::from(\"76a914010101010101010101010101010101010101010188ab\")"
))]
pub slashing_pk_script: String,
/// `min_slashing_tx_fee_sat` is the minimum amount of tx fee (quantified in Satoshi) needed for
/// the pre-signed slashing tx
#[derivative(Default(value = "1000"))]
pub min_slashing_tx_fee_sat: u64,
/// `slashing_rate` determines the portion of the staked amount to be slashed,
/// expressed as a decimal (e.g. 0.5 for 50%).
#[derivative(Default(value = "String::from(\"0.1\")"))]
pub slashing_rate: String,
}
).

This may be good for simplicity, but it's bad for ergonomics. If we want override a single default value, we need to provide the entire Config / Params struct fields (if not, serde will complain during deserialisation).

So, it's better to make those config / params fields explicit, flattening them in the InstantiationMsg directly. That way they are visible, settings are clear and well documented, there's no surprising / default behaviour, and less eventual head scratching.

We can still use default values through derivative or helpers, but let's restrict those to tests, and test helpers.

This will be breaking contract instantiation in a number of places (e2e tests, integration scripts, etc.).
But, that's better than implicitly setting undesired / unknown default values.

Regarding Config vs. Params, we could simplify that as well putting everything under the same struct. But I think the separation has some merit: config values are those immutable values gathered during instantiation, whereas we can use params for mutable / settable values (through governance, administrator accounts, etc.)

@SebastianElvis
Copy link
Member

Good point, we should do this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants