-
Notifications
You must be signed in to change notification settings - Fork 51
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
Update dependencies to 0.9.43 #137
Conversation
On hold until we fix the issue with the p2p right¿ |
To silence warning about weight 0
This PR now includes #142 , should fix the peer discovery bug |
type FreezeIdentifier = (); | ||
type MaxFreezes = (); | ||
type HoldIdentifier = (); | ||
type MaxHolds = (); |
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.
It is ok to use the default values for these?
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.
yes for now yes
@@ -951,10 +968,23 @@ impl_runtime_apis! { | |||
} | |||
|
|||
fn elasticity() -> Option<Permill> { | |||
None | |||
Some(pallet_base_fee::Elasticity::<Runtime>::get()) |
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.
This isn't needed for the upgrade, but I noticed it was set like that in the frontier template, and this compiles. So need someone to confirm whether we want this to Some or None.
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.
it really depends. But as long as base-fee is there you can leave it like this
# TODO: we enable this feature here, but this feature should not be enabled when compiling | ||
# the other substrate-based runtimes. Check if cargo does enabled it automatically because | ||
# it probably does (otherwise it would have to compile this crate twice) | ||
pallet-balances = { workspace = true, features = ["insecure_zero_ed"] } |
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 verified this, and it depends on how we run cargo build
. With our current CI setup, both templates are built at the same time using one cargo command, so both templates have this feature enabled.
Building them separately like this, the feature is only enabled in the frontier template:
cargo build --release --features=runtime-benchmarks -p container-chain-template-simple-node
cargo build --release --features=runtime-benchmarks -p container-chain-template-frontier-node
But I don't think this is an issue, because the only differences when compiling with this feature are:
- Disable an assert that the existential deposit is not zero
- Change the impl of have_providers_or_no_zero_ed from always returning true to doing some computation (storage read?)
/// Returns `true` when `who` has some providers or `insecure_zero_ed` feature is disnabled.
/// Returns `false` otherwise.
#[cfg(not(feature = "insecure_zero_ed"))]
fn have_providers_or_no_zero_ed(_: &T::AccountId) -> bool {
true
}
/// Returns `true` when `who` has some providers or `insecure_zero_ed` feature is disnabled.
/// Returns `false` otherwise.
#[cfg(feature = "insecure_zero_ed")]
fn have_providers_or_no_zero_ed(who: &T::AccountId) -> bool {
frame_system::Pallet::<T>::providers(who) > 0
}
@@ -371,6 +375,7 @@ parameter_types! { | |||
impl pallet_sudo::Config for Runtime { | |||
type RuntimeCall = RuntimeCall; | |||
type RuntimeEvent = RuntimeEvent; | |||
type WeightInfo = (); |
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 should probably bring the weights calculated by substrate
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 thought that ()
is equivalent to the default weights, which are the ones calculated by substrate?
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 dont know for sure, I always thought it does not apply any weight.
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.
They are equivalent, but it says that ()
is for backwards compatibility and tests, so maybe we should change it in the future
Peer discovery bug doesn't affect us anymore thanks to #142, and also I have cherry-picked a fix from upstream here: moondance-labs/substrate@11f2f27