Skip to content

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

Merged
mnaamani merged 3 commits intoJoystream:narafrom
freakstatic:nara-freeze-pallet
Oct 23, 2023
Merged

[Nara] Add freeze pallet extrinsic to CRT pallet#4922
mnaamani merged 3 commits intoJoystream:narafrom
freakstatic:nara-freeze-pallet

Conversation

@freakstatic
Copy link
Copy Markdown
Contributor

@freakstatic freakstatic commented Oct 9, 2023 •
edited by sync-by-unito bot
Loading

Implements the freeze part of #4897

┆Issue is synchronized with this Asana task by Unito

@freakstatic
Copy link
Copy Markdown
Contributor Author

freakstatic commented Oct 9, 2023

@mnaamani can you please review?

@freakstatic freakstatic force-pushed the nara-freeze-pallet branch 3 times, most recently from b4faf62 to 6061631 Compare October 11, 2023 22:15
dobertRowneySr
dobertRowneySr suggested changes Oct 17, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@freakstatic
Copy link
Copy Markdown
Contributor Author

freakstatic commented Oct 17, 2023 •
edited
Loading

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
#4897 (comment)

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
Copy link
Copy Markdown
Contributor Author

freakstatic commented Oct 17, 2023

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

@freakstatic freakstatic force-pushed the nara-freeze-pallet branch 2 times, most recently from 3e2eaa4 to 5c7a021 Compare October 18, 2023 00:43
@dobertRowneySr dobertRowneySr self-requested a review October 18, 2023 08:51
dobertRowneySr
dobertRowneySr approved these changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr left a comment

Choose a reason for hiding this comment

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

LGTM

mnaamani
mnaamani requested changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Great contribution, just left some change requests and general comments on use of assert_noop and unit tests.

@freakstatic freakstatic force-pushed the nara-freeze-pallet branch 4 times, most recently from e34cb8f to 6cf3c1b Compare October 21, 2023 11:06
mnaamani
mnaamani requested changes Oct 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

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

@freakstatic
Copy link
Copy Markdown
Contributor Author

freakstatic commented Oct 23, 2023

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

@mnaamani mnaamani self-requested a review October 23, 2023 15:09
@mnaamani mnaamani merged commit d86099c into Joystream:nara Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mnaamani mnaamani mnaamani approved these changes

+1 more reviewer

@dobertRowneySr dobertRowneySr dobertRowneySr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@freakstatic @mnaamani @dobertRowneySr