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.

Sign up for GitHub

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

add marketplace upgrade type #1850

Merged
merged 9 commits into from
Aug 14, 2024
Merged

add marketplace upgrade type #1850

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4be73e7
marketplace upgrade type
imabdulbasit Aug 1, 2024
5889cf8
Merge branch 'main' into ab/marketplace-upgrade-type
imabdulbasit Aug 12, 2024
545bcbc
fix version in toml file
imabdulbasit Aug 13, 2024
91ab69c
use empty variant for marketplace
imabdulbasit Aug 13, 2024
e6cf990
rename chainconfig upgrade type to feeupgrade
imabdulbasit Aug 14, 2024
8d7de78
fix tests and update doc
imabdulbasit Aug 14, 2024
849d54c
Merge branch 'main' into ab/marketplace-upgrade-type
imabdulbasit Aug 14, 2024
2a0f407
rename fee_upgrade to fee
imabdulbasit Aug 14, 2024
b1c0a47
remove comment
imabdulbasit Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions data/genesis/demo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,10 @@ base_fee = '1 wei'
max_block_size = '1mb'
fee_recipient = '0x0000000000000000000000000000000000000000'
fee_contract = '0xa15bb66138824a1c7167f5e85b957d04dd34e468'

[[upgrade]]
version = "0.3"
start_proposing_view = 5
stop_proposing_view = 15

[upgrade.marketplace]
43 changes: 39 additions & 4 deletions sequencer/src/genesis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,8 @@ impl Genesis {
let upgrades: Vec<&Upgrade> = self.upgrades.values().collect();

for upgrade in upgrades {
match upgrade.upgrade_type {
UpgradeType::ChainConfig { chain_config } => {
base_fee = std::cmp::max(chain_config.base_fee, base_fee);
}
if let UpgradeType::ChainConfig { chain_config } = upgrade.upgrade_type {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this Vec<&Upgrade> holds the actions we need to perform per type per upgrade? Is that right? So we are saying that for every upgrade we need to set base_fee to the maximum base fees of all upgrades. I think implicit in this is that all base_fee can only ever increment. Just checking my understanding. thanks!

Copy link
Contributor Author

@imabdulbasit imabdulbasit Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vec<Upgrade> is basically array of tables from the genesis toml file. Yes, we set base fee to max base fee if genesis toml file has fee upgrades (chain config upgrades) specified. This is required by builder as builder takes base_fee parameter which can not be updated as it doesn't know about validated state

base_fee = std::cmp::max(chain_config.base_fee, base_fee);
}
}

Expand Down Expand Up @@ -571,4 +569,41 @@ mod test {

toml::from_str::<Genesis>(&toml).unwrap_err();
}

#[test]
fn test_marketplace_upgrade_toml() {
let toml = toml! {
[stake_table]
capacity = 10

[chain_config]
chain_id = 12345
max_block_size = 30000
base_fee = 1
fee_recipient = "0x0000000000000000000000000000000000000000"
fee_contract = "0x0000000000000000000000000000000000000000"

[header]
timestamp = 123456

[accounts]
"0x23618e81E3f5cdF7f54C3d65f7FBc0aBf5B21E8f" = 100000
"0x0000000000000000000000000000000000000000" = 42

[l1_finalized]
number = 64
timestamp = "0x123def"
hash = "0x80f5dd11f2bdda2814cb1ad94ef30a47de02cf28ad68c89e104c00c4e51bb7a5"

[[upgrade]]
version = "0.4"
start_proposing_view = 1
stop_proposing_view = 10

[upgrade.marketplace]
}
.to_string();

toml::from_str::<Genesis>(&toml).unwrap();
}
}
20 changes: 8 additions & 12 deletions types/src/v0/impls/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,13 +747,11 @@ impl BlockHeader<SeqTypes> for Header {
let mut validated_state = parent_state.clone();

let chain_config = if version > instance_state.current_version {
match instance_state
.upgrades
.get(&version)
.map(|upgrade| match upgrade.upgrade_type {
match instance_state.upgrades.get(&version) {
Some(upgrade) => match upgrade.upgrade_type {
UpgradeType::ChainConfig { chain_config } => chain_config,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not looking for a change here, just trying to improve my understanding. These semantics are confusing to me. Why do we have an UpgradeType:ChainConfig. Couldn't we just be upgrading between versions, and have the version define what is included in the upgrade? For example, I would imaging that the upgrade type would be Marketplace and ChainConfig upgrade would happen "under the hood" since it is a dependency of marketplace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think renaming this upgrade type to FeeUpgrade would be better. So, fee upgrade would require a new chain config that is provided in the toml file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! yes I think so too.

}) {
Some(cf) => cf,
_ => Header::get_chain_config(&validated_state, instance_state).await,
},
None => Header::get_chain_config(&validated_state, instance_state).await,
}
} else {
Expand Down Expand Up @@ -866,13 +864,11 @@ impl BlockHeader<SeqTypes> for Header {
let mut validated_state = parent_state.clone();

let chain_config = if version > instance_state.current_version {
match instance_state
.upgrades
.get(&version)
.map(|upgrade| match upgrade.upgrade_type {
match instance_state.upgrades.get(&version) {
Some(upgrade) => match upgrade.upgrade_type {
UpgradeType::ChainConfig { chain_config } => chain_config,
}) {
Some(cf) => cf,
_ => Header::get_chain_config(&validated_state, instance_state).await,
},
None => Header::get_chain_config(&validated_state, instance_state).await,
}
} else {
Expand Down
6 changes: 2 additions & 4 deletions types/src/v0/impls/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,8 @@ impl ValidatedState {
return;
};

match upgrade.upgrade_type {
UpgradeType::ChainConfig { chain_config } => {
self.chain_config = chain_config.into();
}
if let UpgradeType::ChainConfig { chain_config } = upgrade.upgrade_type {
self.chain_config = chain_config.into();
}
}

Expand Down
3 changes: 2 additions & 1 deletion types/src/v0/v0_1/instance_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ use crate::{v0_3::ChainConfig, Timestamp};

/// Represents the specific type of upgrade.
#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
#[serde(untagged)]
#[serde(rename_all = "snake_case")]
#[serde(untagged)]
pub enum UpgradeType {
// Note: Wrapping this in a tuple variant causes deserialization to fail because
// the 'chain_config' name is also provided in the TOML input.
ChainConfig { chain_config: ChainConfig },
Marketplace {},
}

/// Represents an upgrade based on time (unix timestamp).
Expand Down
Loading