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

Data Preservers Assignments #563

Merged
merged 37 commits into from
Jun 12, 2024
Merged

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented May 29, 2024

Modifies the pallet to support assignments.

TODO in this PR:

  • Update runtime configs to support only free assignment for now
  • Benchmark the new extrinsics
  • Write migration from old Bootnodes storage to new Profiles + Assignements

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Everything looks good to me so far. Let's start configuring runtimes and migrations for this

@nanocryk nanocryk marked this pull request as ready for review June 5, 2024 09:46
@nanocryk nanocryk added a0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 5, 2024
.rustfmt.toml Outdated
max_width = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed imports_granularity = "One" which requires nightly and is apparently only used by me haha

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry for not using it, but I still think it has some value, similar to clippy warnings which we fix once in a while. And if you want to remove it it's better to open a separate PR


const profileId = await api.query.dataPreservers.nextProfileId();
txs.push(
api.tx.dataPreservers.createProfile({
Copy link
Contributor

Choose a reason for hiding this comment

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

In all zombienet tests we should use forceCreateProfile, copy it from test/suites/parathreads/test_tanssi_parathreads.ts

This currently fails if you run

pnpm moonwall test zombie_tanssi_one_node

Copy link
Collaborator

Choose a reason for hiding this comment

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

it depends how we do it though, but in general yes, any batch call executed with sudo should do that

Copy link
Contributor

Choose a reason for hiding this comment

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

It was only this test that was failing, I fixed it and pushed a commit

Copy link
Contributor

Choose a reason for hiding this comment

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

This file doesn't compile with --features=try-runtime but I believe it's also broken in master, add a

use sp_runtime::DispatchError

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes please! thank you tomasz

}

/// Implementation of `ProfileDeposit` based on the size of the SCALE-encoding.
pub struct BytesProfileDeposit<BaseCost, ByteCost>(PhantomData<(BaseCost, ByteCost)>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fairly generic ( you only have Profile that requires an encode) I am surprised substrate does not provide anyhting like this alreay. But in any case perhaps this does not belong here but rather somewhere more generic, so that it can be re-used by other pallets. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found that substrate has something similar: https://github.com/paritytech/polkadot-sdk/blob/b65313e81465dd730e48d4ce00deb76922618375/substrate/frame/support/src/traits/storage.rs#L183

Essentially you can define a FootPrint structure with the count and len of profile

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -37,12 +38,14 @@ describeSuite({
const storedProfile = await polkadotJs.query.dataPreservers.profiles(profileId);
expect(storedProfile.toJSON()).to.be.deep.equal({
account: general_user_bob.address,
deposit: 10_190_000_000_000,
deposit: 10_200_000_000_000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add start_assignemnt and force_start_assignment as well as stop_assignment tets here?

@@ -120,22 +146,33 @@ pub mod pallet {
+ Debug
+ Eq
+ CheckedAdd
+ One;
+ One
+ Ord;

// Who can call set_boot_nodes?
type SetBootNodesOrigin: EnsureOriginWithArg<Self::RuntimeOrigin, ParaId>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I'll remove it :)

@nanocryk nanocryk merged commit f6bb1f9 into master Jun 12, 2024
34 checks passed
@nanocryk nanocryk deleted the jeremy-data-preservers-assignements branch June 12, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants