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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Oct 23, 2023
Merged

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

merged 3 commits into from
Oct 23, 2023

Conversation

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

@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
ignazio-bovo
ignazio-bovo suggested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@ignazio-bovo ignazio-bovo 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

runtime-modules/project-token/src/lib.rs Outdated Show resolved Hide resolved
runtime-modules/project-token/src/lib.rs Outdated Show resolved Hide resolved
runtime-modules/project-token/src/events.rs Show resolved Hide resolved
runtime-modules/project-token/src/traits.rs Outdated Show resolved Hide resolved
@freakstatic
Copy link
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
Contributor Author

@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
@ignazio-bovo ignazio-bovo self-requested a review October 18, 2023 08:51
ignazio-bovo
ignazio-bovo approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@ignazio-bovo ignazio-bovo 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
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.

runtime-modules/common/src/lib.rs Outdated Show resolved Hide resolved
runtime-modules/content/src/tests/creator_tokens/finalize_creator_token_sale.rs Outdated Show resolved Hide resolved
runtime-modules/project-token/src/events.rs Outdated Show resolved Hide resolved
runtime-modules/content/src/tests/creator_tokens/claim_patronage_credit.rs Show resolved Hide resolved
@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
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

runtime-modules/project-token/src/lib.rs Outdated Show resolved Hide resolved
@freakstatic
Copy link
Contributor Author

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
23 of 24 checks passed
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

@ignazio-bovo ignazio-bovo ignazio-bovo approved these changes

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 @ignazio-bovo