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

fix: transaction pause bounded storage #641

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Conversation

Roznovjak
Copy link
Contributor

This PR removes without_storage_info from the Transaction pause pallet and adds a storage migration.

@github-actions
Copy link

github-actions bot commented Jul 14, 2023

Crate versions that have not been updated:

  • hydradx-runtime: v169.0.0

Crate versions that have been updated:

  • pallet-transaction-pause: v0.1.3 -> v1.0.0

Runtime version has not been increased.

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 71.42% and project coverage change: +0.04% 🎉

Comparison is base (e2c4be3) 64.77% compared to head (5546cac) 64.81%.
Report is 1 commits behind head on master.

❗ Current head 5546cac differs from pull request most recent head 136dbea. Consider uploading reports for the commit 136dbea to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   64.77%   64.81%   +0.04%     
==========================================
  Files         134      135       +1     
  Lines        9749     9800      +51     
==========================================
+ Hits         6315     6352      +37     
- Misses       3434     3448      +14     
Files Changed Coverage Δ
runtime/hydradx/src/lib.rs 3.33% <ø> (ø)
runtime/hydradx/src/migrations.rs 0.00% <0.00%> (ø)
pallets/transaction-pause/src/lib.rs 84.21% <90.90%> (+0.87%) ⬆️
pallets/transaction-pause/src/migration.rs 90.90% <90.90%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

hope I'm not being too nitpicky

pallets/transaction-pause/src/migration.rs Outdated Show resolved Hide resolved
let mut weight = Weight::zero();

let status = v0::PausedTransactions::<T>::drain().collect::<Vec<_>>();
weight.saturating_accrue(T::DbWeight::get().reads(status.len() as u64));
Copy link
Contributor

Choose a reason for hiding this comment

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

should be one read, one write, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't find these numbers in the docs. I'm just curious, how do you know it's 1w/1r? Thanks.


StorageVersion::new(1).put::<Pallet<T>>();

T::DbWeight::get().reads_writes(1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

you accumulate weight, but then don't return it

continue;
}

PausedTransactions::<T>::insert((pallet_name_b.unwrap(), function_name_b.unwrap()), ());
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: write with expect or rewrite the block to not use unwrap
e.g. with match statement like:

match (pallet_name_b, function_name_b) {
    (Some(pallet), Some(function)) => insert(...),
    _ => log::info!(...)
}

log::info!(
target: TARGET,
"Value not migrated because it's too long: {:?}",
(pallet_name_b, function_name_b)
Copy link
Contributor

Choose a reason for hiding this comment

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

probably want to print the unbounded names?

@mrq1911 mrq1911 changed the title feat: transaction pause bounded storage fix: transaction pause bounded storage Jul 27, 2023
@Roznovjak Roznovjak requested a review from apopiak July 27, 2023 19:46
@mrq1911 mrq1911 merged commit 809d38e into master Jul 27, 2023
3 of 4 checks passed
@Roznovjak Roznovjak deleted the tx_pause_bounded_storage branch July 28, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants