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

feat: add block number for CTypes #440

Merged
merged 29 commits into from
Jan 23, 2023
Merged

feat: add block number for CTypes #440

merged 29 commits into from
Jan 23, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 29, 2022

Fixes https://github.com/KILTprotocol/ticket/issues/2327 and fixes https://github.com/KILTprotocol/ticket/issues/2325.

Adds a creation block number to future CTypes, and it introduces a set_block_number extrinsic that can be called by sudo on standalone and Peregrine, and by 3/5ths of the technical committee on Spiritnet (this can be changed if we agree to). I personally think that involving (part of) the technical committee is the right compromise between agility of action and decentralization. An alternative, more complex solution would be to implement an RPC function that verifies, given the block number and the ctype hash, whether there was a tx in that block for the given CType. This would be cool, and technically could also be feeless, but I am not sure it's worth the hassle.

@ntn-x2 ntn-x2 self-assigned this Nov 29, 2022
@ntn-x2 ntn-x2 marked this pull request as ready for review November 30, 2022 12:32
Copy link
Contributor

@weichweich weichweich left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few points that I would like to get clarified.

pallets/ctype/src/ctype_entry.rs Outdated Show resolved Hide resolved
@@ -553,6 +553,8 @@ impl delegation::Config for Runtime {
type Deposit = constants::delegation::DelegationDeposit;
}

type CTypeBlockNumberSetOrigin = pallet_collective::EnsureProportionAtLeast<AccountId, TechnicalCollective, 3, 5>;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would argue that we should do that using a general vote and not the TC.

Copy link
Contributor

Choose a reason for hiding this comment

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

By general vote you are referring to a referendum or the Council collective? I agree we shouldnt give special power to special entities.

Copy link
Contributor

Choose a reason for hiding this comment

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

referendum

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why the general public would be concerned about setting the preimage for a CType hash. Yes, it will probably be a one-off operation, but I don't see why we want to wait 2+ weeks to do something that only has technical motivations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the block number is set automatically, you would only need to trust the blockchain, that the number is correct. So trust the chain == trust that polkadot checks everything and the KILT governance(all token holders) is honest and fair.
If you allow the TC to set the block number you would need to trust them, that they set the correct number. It feels weaker.
Also since this should only be necessary once after the upgrade, i would say it's fine to wait 2 weeks.

Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Adding to Albi's remarks, I would do some minor changes to the migration.

pallets/ctype/src/benchmarking.rs Show resolved Hide resolved
pallets/ctype/src/lib.rs Outdated Show resolved Hide resolved
runtimes/common/src/migrations.rs Outdated Show resolved Hide resolved
runtimes/common/src/migrations.rs Show resolved Hide resolved
runtimes/common/src/migrations.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@weichweich weichweich left a comment

Choose a reason for hiding this comment

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

LGTM

I don't have a strong opinion for governance issue. If you are not convinced by the arguments, I'm also fine with leaving it like it is.

@ntn-x2
Copy link
Member Author

ntn-x2 commented Jan 3, 2023

If you are not convinced by the arguments, I'm also fine with leaving it like it is.
I understood your point, and I will go over your concern again later today or tomorrow. It might be right to do what you are suggesting.

William was suggesting we want to have a migration strategy for CTypes that mirrors what the Ethereum migration does. Do we still want to do that? I think in this case is probably unnecessary, but it is also true that this feature is not super high priority and could wait.

@weichweich
Copy link
Contributor

If you are not convinced by the arguments, I'm also fine with leaving it like it is.
I understood your point, and I will go over your concern again later today or tomorrow. It might be right to do what you are suggesting.

William was suggesting we want to have a migration strategy for CTypes that mirrors what the Ethereum migration does. Do we still want to do that? I think in this case is probably unnecessary, but it is also true that this feature is not super high priority and could wait.

I think it's not necessary. The current strategy is fine.

@weichweich weichweich enabled auto-merge (squash) January 23, 2023 15:15
@weichweich weichweich merged commit 0a5e46d into develop Jan 23, 2023
@weichweich weichweich deleted the aa/ctype-block-number branch January 23, 2023 15:35
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