Rome version of content working group by bedeho · Pull Request #87 · Joystream/joystream

bedeho · 2019-10-29T17:22:58Z

Specification

#107

Progress

Future

update_curator_application_role_account: switch out account using membership, in case key was lost?

Dropped

These were all dropped when the principal model was simplified.

update_lead_credential
update_any_member_credential
update_any_curator_credential
create_dynamic_credential
update_dynamic_credential

Comments

There is significant effort involved in always checking whether a given member can or cannot step into a given role at some point in time, and then later to actually move them in and out of that role. In some cases, there may be a double check, such as the initial check when someone applies for a position, and then a second one when they are hired by the lead. All of this takes effort to manage, and mostly it exists so that one can model very fine-grained rules about who can step into what role at what time. We should at a later time reconsider the value of this because currently, we don't even have any policy in mind, it's just a blanket yes for everything.

Missing

Integrity check on channel ownership in memership module #96
Constraining max number of applications and openings. Not only simultaneous, but also cumulative, because this is not cleaned up in hiring/staking: Allow optional channel fields to have an empty vector as a value #126 (comment).

mnaamani · 2019-10-31T08:35:55Z

@bedeho the build is currently failing because we need to update the dependencies to use the same commit hash for substrate, as well as some minor tweaks to fix issues that only show up when building for WASM. (specifically use of Vec)

bedeho · 2019-11-01T10:59:41Z

Great that you are on top of it, I have a temporary fix for the Vec issue here

Joystream/substrate-versioned-store-permissions-module#3

so I am not blocked by that.

bedeho · 2019-11-02T12:12:32Z

I know get this build error when doing cargo test

error[E0277]: `substrate_versioned_store::Property` doesn't implement `core::fmt::Debug`
   --> /Users/bedeho/.cargo/git/checkouts/substrate-versioned-store-permissions-module-9fdc387c352881a2/694a504/src/lib.rs:103:1
    |
103 | / decl_module! {
104 | |     pub struct Module<T: Trait> for enum Call where origin: T::Origin {
105 | |
106 | |         /// Sets the admins for a class
...   |
385 | |     }
386 | | }
    | |_^ `substrate_versioned_store::Property` cannot be formatted using `{:?}` because it doesn't implement `core::fmt::Debug`
    |
    = help: the trait `core::fmt::Debug` is not implemented for `substrate_versioned_store::Property`
    = note: required because of the requirements on the impl of `core::fmt::Debug` for `sr_api_hidden_includes_decl_storage::hidden_include::sr_primitives::sr_std::prelude::Vec<substrate_versioned_store::Property>`
    = note: required because of the requirements on the impl of `core::fmt::Debug` for `(core::option::Option<<T as Trait>::PrincipalId>, u64, sr_api_hidden_includes_decl_storage::hidden_include::sr_primitives::sr_std::prelude::Vec<u16>, sr_api_hidden_includes_decl_storage::hidden_include::sr_primitives::sr_std::prelude::Vec<substrate_versioned_store::Property>)`
    = note: required by `core::fmt::Debug::fmt`
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

Property only auto-supports Debug trait through

#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]

which only includes the derives when we are not building the WASM target?

I hope we can have a more safe and sound solution to resolve this issue of things breaking when they are used in ways that are not so visible when they are built and run on their own.

Plus lots of supporting apparatus usable in other extrinsics later

A new way to ensure that an origin is signed by the controller account of a member

Update to use ensure_member_controller_account_signed.

Is used in transfer_channel_ownership

- Update name - Enforce availability of name

A new utility routine for updating channel properties

mnaamani

Really clean and surprisingly easy to go through, despite the number of modules that come together in this module.

mnaamani · 2019-12-03T09:26:22Z

+    trait Store for Module<T: Trait> as ContentWorkingGroup {
+
+        /// The mint currently funding the rewards for this module.
+        pub Mint get(mint) config(): <T as minting::Trait>::MintId;


we don't have configurable mints at genesis.. so setting a mintid at genesis is problematic atm.
Either we can have this be an Option<MintId> with the mint getting created later with a sudo call,
or my preferred way to use a build() with some extra_genesis values to create a new mint.

It also just occurred to me, I don't see the mint id being used when creating reward relationships.

I see now the relationship is created through an extrinsic where the mintid is provided..

Here is one approach to create mint with build() at genesis: 438c031

mnaamani · 2019-12-03T09:28:12Z

+
+            Self::update_channel(
+                &channel_id,
+                &None,


new_channel_name isn't used to update

Suggested change

&None,

&new_channel_name,

mnaamani · 2019-12-03T09:32:00Z

+        pub fn fill_curator_opening(
+            origin,
+            curator_opening_id: CuratorOpeningId<T>,
+            successful_curator_application_ids: BTreeSet<CuratorApplicationId<T>>


Does an empty set need to be handled in a special way?
Would have to look at behavior of hiring module when calling fill_opening with empty application set.

mnaamani · 2019-12-03T09:53:26Z

+            stake_id: StakeId<T>
+        ) {
+            // Ensure its a runtime root origin
+            ensure_root(origin)?;


This can be called via sudo extrinsic. I don't think this is what you intended. Its probably the method that will be called from handler for StakingEventHandler::unstaked ?

No values are ever inserted into UnstakerByStakeId map. Is that step missing in apply_on_curator_opening()

Correction.. (as per chat) step is missing in deactivate_curator()

Yes this is a bug, we must add to this map whenever there is async unstaking initaited, that is staking with an unboding period.

Here is a commit from #103 which makes unstaked a normal method: 92ef426

mnaamani · 2019-12-06T08:16:04Z

+            member_id: T::MemberId,
+            curator_opening_id: CuratorOpeningId<T>,
+            role_account: T::AccountId,
+            source_account: T::AccountId,


There are no guarantees that the origin controls the funds in the source_account provided, which means they can stake funds from arbitrary accounts.

In the storage role staking module we split this staking into a two step process but I found that a little too complex both on the runtime and ui side.

So maybe we can opt for something simpler here, for example that the source account must be the member's root_account or controller_account?

Well spotted, quite a severe oversight, I don't love the remedy, but may be best to do for Rome, and fix properly later.

mnaamani · 2019-12-09T08:11:42Z

I think we need to add some extrinsics to give sudo ability to update the mint capacity, and or adjustment policy.

- Update From<WrappedError<AddOpeningError>>: add message for the new error type - ApplicationRationingZeroMaxApplicants - Cover From<WrappedError<AddOpeningError>> with tests

- update hiring-module dependency revision - update stake-module dependency to restore compatibility with hiring module - both dependencies are latest development revision to date

Hiring module update support

mnaamani · 2020-01-08T11:29:38Z

merging, but there are pending issues to be fixed. see #117

Link to setup guide

bedeho and others added 4 commits October 29, 2019 18:18

wip

d56916f

wip

7fb49ca

wip

056cb23

wip

66908bc

wip

6d5cfa5

Consuming new fixed dependencies

64d68c5

bedeho and others added 21 commits November 2, 2019 15:04

Started initial work on extrinsics, just sketching

9ad9520

Main lib builds, only tests missing some trait impls

f638ed3

Almost building, pushing for review

9c8afcf

Finally builds

409e5dc

Cleanup warnings

7fd4259

create_channel almost done

b6224a5

Implemented transfer_channel_ownership

bd3a30b

Implemented set_lead

b317e16

Missing parts of lead_set

5d1aff9

Merge branch 'role_checking_fixed' into add-content-wg

1ede1fe

Merge branch 'role_checking_fixed' into add-content-wg

6a20edc

Use new hiring module dependency

44b079f

Implementing add_curator_opening

109729d

Plus lots of supporting apparatus usable in other extrinsics later

Fix to use new version of membership interface

9074cb0

Drop on_finalized

ecc9de6

Cleanup

f3a7bde

Implement unset_lead

c300b68

Completing accept_curator_applications

15471da

Complete begin_curator_applicant_review

094a320

Implemented fill_curator_opening

61c7395

Implemented terminate_curator

9e6fcce

bedeho and others added 7 commits November 28, 2019 18:32

Added ensure_member_controller_account_signed

94cd085

A new way to ensure that an origin is signed by the controller account of a member

Implemented update_curator_role_account

45cf9cc

Implemented update_curator_reward_account

a8754f7

Update apply_on_curator_opening

53082e3

Update to use ensure_member_controller_account_signed.

Compiler warning cleanup

8839dc2

Fix comment

52af731

Implemented update_channel_curation_status

b3c7ad7

bedeho added the mod: content-wg label Nov 29, 2019

bedeho added 6 commits November 29, 2019 12:20

Updating incorrect comment

3c73381

Introducing ensure_channel_owner_signed

37d2d08

Is used in transfer_channel_ownership

Update ensure_channel_handle_is_valid

16332f5

- Update name - Enforce availability of name

Introduced update_channel

ebdce02

A new utility routine for updating channel properties

Implemented update_channel_as_curation_actor

df2393c

Implemented update_channel_as_owner

7de27ad

bedeho marked this pull request as ready for review November 29, 2019 11:35

bedeho requested a review from mnaamani November 29, 2019 11:35

bedeho added 2 commits November 29, 2019 12:49

Rustfmt

981de8d

Fixed bug: must remove old mapping when changing channel name

e561ac9

mnaamani requested changes Dec 3, 2019

View reviewed changes

mnaamani reviewed Dec 3, 2019

View reviewed changes

mnaamani reviewed Dec 6, 2019

View reviewed changes

shamil-gadelshin and others added 3 commits December 26, 2019 19:44

Support hiring module changes

ec05e95

- Update From<WrappedError<AddOpeningError>>: add message for the new error type - ApplicationRationingZeroMaxApplicants - Cover From<WrappedError<AddOpeningError>> with tests

Update cargo.toml & cargo.lock

6854c0b

- update hiring-module dependency revision - update stake-module dependency to restore compatibility with hiring module - both dependencies are latest development revision to date

Merge pull request #2 from shamil-gadelshin/hiring_module_update

bfa9bee

Hiring module update support

mnaamani mentioned this pull request Jan 8, 2020

Pending Fixes For Content Working Group #117

Closed

mnaamani merged commit fb4ecf3 into Joystream:development Jan 8, 2020

mnaamani mentioned this pull request Jan 13, 2020

content working group: create mint and optionaly lead at genesis #119

Merged

mnaamani pushed a commit to mnaamani/joystream that referenced this pull request Jun 19, 2020

Merge pull request Joystream#87 from mnaamani/link-to-setup-guide

e588a0b

Link to setup guide

Conversation

bedeho commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Specification

Progress

Future

Dropped

Comments

Missing

Uh oh!

mnaamani commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedeho commented Nov 1, 2019

Uh oh!

bedeho commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnaamani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani commented Dec 9, 2019

Uh oh!

mnaamani commented Jan 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bedeho commented Oct 29, 2019 •

edited

Loading

mnaamani commented Oct 31, 2019 •

edited

Loading

bedeho commented Nov 2, 2019 •

edited

Loading

mnaamani Dec 9, 2019 •

edited

Loading