-
Notifications
You must be signed in to change notification settings - Fork 132
Ambassador Configurations #736
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
base: main
Are you sure you want to change the base?
Conversation
/// | ||
/// The value of 21 comes from the initial OpenGov proposal: <https://github.com/polkadot-fellows/runtimes/issues/264> | ||
pub struct AmbassadorMemberCount; | ||
impl MaybeConvert<Rank, MemberIndex> for AmbassadorMemberCount { | ||
fn maybe_convert(rank: Rank) -> Option<MemberIndex> { | ||
(rank == 3).then_some(21) | ||
(rank == 6).then_some(21) |
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.
There isn't such a limit in the latest manifesto
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.
noted
system-parachains/collectives/collectives-polkadot/src/ambassador/migration.rs
Outdated
Show resolved
Hide resolved
system-parachains/collectives/collectives-polkadot/src/ambassador/mod.rs
Show resolved
Hide resolved
@@ -131,29 +124,16 @@ impl pallet_ranked_collective::Config<AmbassadorCollectiveInstance> for Runtime | |||
#[cfg(feature = "runtime-benchmarks")] |
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.
Do you want to enable some promotion logic or wait for it until we have a smart contract?
#[cfg(feature = "runtime-benchmarks")] | ||
type MinRankOfClass = tracks::MinRankOfClass; | ||
type VoteWeight = Geometric; | ||
type AddOrigin = OpenGovOrGlobalHead; |
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.
Additional Ambassadors will not be able to join until Polkadot Hub is ready and Jesse can build a smart contract to allow the joining process
Should we maybe leave this blank?
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.
AddOrigin
& ExchangeOrigin
can be?
pub type RootOrOpenGov = EitherOfDiverse<
EnsureRoot<AccountId>,
EnsureXcm<IsVoiceOfBody<GovernanceLocation, FellowshipAdminBodyId>>,
>;
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 reasonable.
type VoteWeight = Geometric; | ||
type AddOrigin = OpenGovOrGlobalHead; | ||
type ExchangeOrigin = OpenGovOrGlobalHead; | ||
type MemberSwappedHandler = crate::AmbassadorCore; | ||
type MaxMemberCount = (); |
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 limit this somehow since its running on a system chain. 512 or 1024 maybe? It can be increased in the future but with the current config it would allow one malicious HA to add millions of accounts and slow down the collectives chain.
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 did not consider this...
Considering the event of a malicious HA, adding members through OpenGov seems like a more general solution?
The limits can still be abused IMO.
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.
Additional Ambassadors will not be able to join until Polkadot Hub is ready and Jesse can build a smart contract to allow the joining process.
Maybe we just disable it for now? (from the gdoc manifesto)
system-parachains/collectives/collectives-polkadot/src/ambassador/mod.rs
Show resolved
Hide resolved
// This list was created by collating community-submitted addresses from an off-chain document source | ||
// https://docs.google.com/spreadsheets/d/1uE5nDKuMZDqlj9q2tvnk_tngyU1Cokl0tQKwSigvJLA/edit?gid=0#gid=0, | ||
// then converting each Polkadot SS58 address to its raw public key (32-byte hex) using: | ||
// `subkey inspect <SS58_ADDRESS>` |
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.
Please add something like this, i put it into the preimages pallet onchain now.
A batched call with two remarks has been persisted onto the Collectives Parachchain preimages.
Hash: `0xb6087d77d3b4b5966b9096b459283ea6f5617f4a96d1b4829191439f96fe3c6b`
Transaction link: https://collectives-polkadot.subscan.io/extrinsic/7035411-2

for _ in 0..desired_rank { | ||
let _ = <RankedCollective<T, I> as RankedMembers>::promote(&who); | ||
// 1 write to `IdToIndex` and `IndexToId` per promotion | ||
weight.saturating_accrue(T::DbWeight::get().writes(2)); |
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.
Could also use the weights of the extrinsics but i dont mind.
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.
Weights updated
system-parachains/collectives/collectives-polkadot/src/ambassador/migration.rs
Show resolved
Hide resolved
} | ||
|
||
/* | ||
/// A `TryMorph` implementation which is designed to convert an aggregate `RuntimeOrigin` |
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.
If we dont need it then you can delete it.
retention @ 11..=15 => retention - 8, | ||
// A promotion vote; the track ID turns out to be 18 more than the minimum required | ||
// rank. | ||
promotion @ 21..=25 => promotion - 18, |
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 think promotion and retention requires votes from the members one rank above - not two (like in the tech fellowship).
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.
These configurations match the Tech Fellowship
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.
Correct, one rank above upwards as we only have 6 ranks.
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.
Ahh I see
promotion @ 21..=25 => promotion - 18, | ||
// A fast promotion vote; the track ID turns out to be 28 more than the minimum required | ||
// rank. | ||
fast_promote @ 31..=33 => fast_promote - 28, |
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.
ditto
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.
Did you fix this? Please close the fixed ones.
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.
pls review
system-parachains/collectives/collectives-polkadot/src/ambassador/tracks.rs
Show resolved
Hide resolved
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.
Looks generally good. I put some comments.
One more Q: Should we set the core-fellowship parameters in a migration as well?
Stuff like demotion_period
(per rank)? Maybe the default zero values are sane - dont know. But it would need to be done otherwise by the ParamsOrigin.
system-parachains/collectives/collectives-polkadot/src/ambassador/migration.rs
Outdated
Show resolved
Hide resolved
This should go on chain as a completely separate release, so the community can vote on this properly. |
Appreciate the point. Given the documented path to date, our reading of OpenGov is that a further on-chain vote isn’t required. I’ll share the timeline and reasoning in the Open Element channel for discussion. |
What documented path? |
There has been a miscommunication / understanding on process here and I am clarifying the Ambassador sentiment before responding on behalf. Ultimately, the Tech Fellowship is the final gatekeeper here, so if we need to have a separate release then are supportive. |
/// - ...with an excess rank of 6 gets 21 votes. | ||
pub struct Geometric; | ||
impl Convert<Rank, Votes> for Geometric { | ||
fn convert(r: Rank) -> Votes { |
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 is causing the failing vote
benchmark
I don't want to be a gatekeeper, but I also don't want to include things that are not wanted. As said, we can do a release that only contains these changes and then we let the voters decide. |
Review required! Latest push from author must always be reviewed |
Ambassador Configurations
This PR implements the revised technical specification for the Ambassador Program. Key changes include:
Core Functionality
Rank System
Membership Management
bump
which can be automated off-chain or throughTask
API.SeniorAmbassador
for onlyAssociateAmbassador
.SeniorAmbassador
for onlyAssociateAmbassador
.Voting System
Treasury & Tipping
TREASURY
origin and track.Tip
origin and track.Implementations