search

Joystream / joystream

Joystream Monorepo
http://www.joystream.org
GNU General Public License v3.0
1.42k stars 115 forks source link

[Nara] Add freeze pallet extrinsic to CRT pallet #4922

Closed freakstatic closed 11 months ago

freakstatic commented 11 months ago

Implements the freeze part of #4897

┆Issue is synchronized with this Asana task by Unito

freakstatic commented 11 months ago

@mnaamani can you please review?

freakstatic commented 11 months ago

I have reviewed the companion issue. But the implementation presented here is different from what's prescribed. Am I missing something? Also it's still possible for a creator issue_token, because there is not frozen-pallet check. Is this desired? What follows below are minor suggestions

@ignazio-bovo thanks for the review 👍 Yeah you are right, the objective was changed, please check https://github.com/Joystream/joystream/issues/4897#issuecomment-1742799791

Regarding issue_token well notice, I forgot to protect that function as well, I fixed that and added a test to ensure that it was working as expected.

freakstatic commented 11 months ago

@ignazio-bovo FYI the proposal part will be submitted in another PR

freakstatic commented 11 months ago

Runtime changes looks good.

Just a general comment. It is generally not recommended to force push changes after a PR is opened and reviews are going on. It will make it harder to do followup reviews as reviewer cannot review new changes and would have to look at the PR as whole again.

As a final step when we make runtime changes is to update the types package. To do this build the node, and run it with joystream-node --dev then run yarn update-chain-metadata This produces the chain-metadata.json file at the root.

Then run build script for types package: yarn workspace @joystream/types build commit changes: types/src/augment/* chain-metadata.json query-node/chain-metadata/2002.json

I apologize for the force push, I wanted to avoid having too many commits for the same feature going to the "master" branch. I will avoid it in the future.

I committed the types packages now, @mnaamani please review again