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

Upgrade initializer should be mandatory #151

Open
wants to merge 5 commits into
base: snowbridge
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions bridges/snowbridge/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions bridges/snowbridge/pallets/outbound-queue/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ mod benchmarks {
command: Command::Upgrade {
impl_address: H160::zero(),
impl_code_hash: H256::zero(),
initializer: Some(Initializer {
initializer: Initializer {
params: [7u8; 256].into_iter().collect(),
maximum_required_gas: 200_000,
}),
},
},
};
let origin = AggregateMessageOrigin::Snowbridge([1; 32].into());
Expand Down
6 changes: 3 additions & 3 deletions bridges/snowbridge/pallets/outbound-queue/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ where
command: Command::Upgrade {
impl_address: H160::zero(),
impl_code_hash: H256::zero(),
initializer: None,
initializer: Default::default(),
},
}
}
Expand All @@ -152,10 +152,10 @@ where
command: Command::Upgrade {
impl_address: H160::zero(),
impl_code_hash: H256::zero(),
initializer: Some(Initializer {
initializer: Initializer {
params: (0..1000).map(|_| 1u8).collect::<Vec<u8>>(),
maximum_required_gas: 0,
}),
},
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion bridges/snowbridge/pallets/outbound-queue/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ fn process_message_yields_on_max_messages_per_block() {
command: Command::Upgrade {
impl_address: Default::default(),
impl_code_hash: Default::default(),
initializer: None,
initializer: Default::default(),
},
}
.encode();
Expand Down
2 changes: 1 addition & 1 deletion bridges/snowbridge/pallets/system/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ mod benchmarks {
RawOrigin::Root,
impl_address,
impl_code_hash,
Some(Initializer { params, maximum_required_gas: 100000 }),
Initializer { params, maximum_required_gas: 100000 },
);

Ok(())
Expand Down
7 changes: 3 additions & 4 deletions bridges/snowbridge/pallets/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ pub mod pallet {
Upgrade {
impl_address: H160,
impl_code_hash: H256,
initializer_params_hash: Option<H256>,
initializer_params_hash: H256,
},
/// An CreateAgent message was sent to the Gateway
CreateAgent {
Expand Down Expand Up @@ -278,7 +278,7 @@ pub mod pallet {
origin: OriginFor<T>,
impl_address: H160,
impl_code_hash: H256,
initializer: Option<Initializer>,
initializer: Initializer,
) -> DispatchResult {
ensure_root(origin)?;

Expand All @@ -287,8 +287,7 @@ pub mod pallet {
Error::<T>::InvalidUpgradeParameters
);

let initializer_params_hash: Option<H256> =
initializer.as_ref().map(|i| H256::from(blake2_256(i.params.as_ref())));
let initializer_params_hash: H256 = blake2_256(initializer.params.as_ref()).into();
let command = Command::Upgrade { impl_address, impl_code_hash, initializer };
Self::send(PRIMARY_GOVERNANCE_CHANNEL, command, PaysFee::<T>::No)?;

Expand Down
28 changes: 10 additions & 18 deletions bridges/snowbridge/pallets/system/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,15 @@ fn upgrade_as_root() {
let address: H160 = [1_u8; 20].into();
let code_hash: H256 = [1_u8; 32].into();

assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, None));
assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, Default::default()));

System::assert_last_event(RuntimeEvent::EthereumSystem(crate::Event::Upgrade {
impl_address: address,
impl_code_hash: code_hash,
initializer_params_hash: None,
initializer_params_hash: hex!(
"0e5751c026e543b2e8ab2eb06099daa1d1e5df47778f7787faab45cdf12fe3a8"
)
.into(),
}));
});
}
Expand All @@ -104,19 +107,10 @@ fn upgrade_as_signed_fails() {
let address: H160 = Default::default();
let code_hash: H256 = Default::default();

assert_noop!(EthereumSystem::upgrade(origin, address, code_hash, None), BadOrigin);
});
}

#[test]
fn upgrade_with_params() {
new_test_ext(true).execute_with(|| {
let origin = RuntimeOrigin::root();
let address: H160 = [1_u8; 20].into();
let code_hash: H256 = [1_u8; 32].into();
let initializer: Option<Initializer> =
Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 });
assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer));
assert_noop!(
EthereumSystem::upgrade(origin, address, code_hash, Default::default()),
BadOrigin
);
});
}

Expand Down Expand Up @@ -607,9 +601,7 @@ fn charge_fee_for_upgrade() {
let origin = RuntimeOrigin::root();
let address: H160 = [1_u8; 20].into();
let code_hash: H256 = [1_u8; 32].into();
let initializer: Option<Initializer> =
Some(Initializer { params: [0; 256].into(), maximum_required_gas: 10000 });
assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, initializer.clone()));
assert_ok!(EthereumSystem::upgrade(origin, address, code_hash, Default::default()));

// assert sovereign_balance does not change as we do not charge for sudo operations
let sovereign_account = sibling_sovereign_account::<Test>(para_id.into());
Expand Down
15 changes: 5 additions & 10 deletions bridges/snowbridge/primitives/core/src/outbound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ mod v1 {
impl_address: H160,
/// Codehash of the implementation contract
impl_code_hash: H256,
/// Optionally invoke an initializer in the implementation contract
initializer: Option<Initializer>,
/// Invoke initializer delegated to the implementation contract
initializer: Initializer,
},
/// Create an agent representing a consensus system on Polkadot
CreateAgent {
Expand Down Expand Up @@ -169,9 +169,7 @@ mod v1 {
ethabi::encode(&[Token::Tuple(vec![
Token::Address(*impl_address),
Token::FixedBytes(impl_code_hash.as_bytes().to_owned()),
initializer
.clone()
.map_or(Token::Bytes(vec![]), |i| Token::Bytes(i.params)),
Token::Bytes(initializer.params.clone()),
])]),
Command::CreateAgent { agent_id } =>
ethabi::encode(&[Token::Tuple(vec![Token::FixedBytes(
Expand Down Expand Up @@ -218,6 +216,7 @@ mod v1 {
/// Representation of a call to the initializer of an implementation contract.
/// The initializer has the following ABI signature: `initialize(bytes)`.
#[derive(Clone, Encode, Decode, PartialEq, RuntimeDebug, TypeInfo)]
#[cfg_attr(feature = "std", derive(Default))]
pub struct Initializer {
/// ABI-encoded params of type `bytes` to pass to the initializer
pub params: Vec<u8>,
Expand Down Expand Up @@ -393,13 +392,9 @@ impl GasMeter for ConstantGasMeter {
AgentExecuteCommand::TransferToken { .. } => 100_000,
},
Command::Upgrade { initializer, .. } => {
let initializer_max_gas = match *initializer {
Some(Initializer { maximum_required_gas, .. }) => maximum_required_gas,
None => 0,
};
// total maximum gas must also include the gas used for updating the proxy before
// the the initializer is called.
50_000 + initializer_max_gas
50_000 + initializer.maximum_required_gas
},
Command::SetTokenTransferFees { .. } => 60_000,
Command::SetPricingParameters { .. } => 60_000,
Expand Down
Loading