Skip to content

Conversation

@dobertRowneySr
Copy link
Collaborator

@dobertRowneySr dobertRowneySr commented Jan 31, 2022

fixes:

I also removed all the unused code like PersonId and the likes that was not removed this far, in order to have cleaner type aliases

Update 2022-02-02: rebased to current olympia . This lead to a larger number of commits
Relevant comits start from : c86b9dd

Relevant code section are in the content module, in particular:

  • NFTIssuanceParameter type introduced
  • InitTransactionalStatus type introduced
  • ensure_valid_init_transactional_status guard added
  • create_video & issue_nft extrinsic argument modified

@vercel
Copy link

vercel bot commented Jan 31, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/joystream/pioneer-testnet/DrgnfQDUqymLYmQCctq2MRypMk1t
✅ Preview: Canceled

[Deployment for a6d433f canceled]

@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from a8043f2 to 0f174e1 Compare February 1, 2022 13:27
@dobertRowneySr
Copy link
Collaborator Author

dobertRowneySr commented Feb 2, 2022

The underlying issues look addressed. However, there are multiple issues with the code:

* no doc-comments for public types, extrinsics, and methods

* please, remove `allow all` clippy warning statement and fix all clippy issues in the pallet

* found this line in the code: `// TODO: Solve #now`

* rebase seems unsuccessful: 1) there is still unrelated code in the PR (removed empty extrinsics), 2) `generate_weights.sh` script fails now (incorrect benchmark name)

* there are public methods with no external usage (should be private)
  • the TODO was an old reference to myself that I solved and forgot to remove
  • benchmark should be fixed
  • removed the cargo directive
  • added doc-comments on types and helper functions added

I could locate the removed_empty_extrinsic reference in the Github commit tab

@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from 5884162 to 89376b3 Compare February 3, 2022 10:50
@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from 89376b3 to af45c7f Compare February 3, 2022 10:51
@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from af45c7f to fe745c8 Compare February 3, 2022 10:52

Self::deposit_event(RawEvent::VideoCategoryDeleted(actor, category_id));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a rebase artifact?

Copy link
Collaborator Author

@dobertRowneySr dobertRowneySr Feb 3, 2022

Choose a reason for hiding this comment

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

I am not following. The BASE commit in olympia has this code. In this PR that I am opening I removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So, it's part of the PR. I was expecting only the NFT-related code.

@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from fe745c8 to 195458d Compare February 3, 2022 12:32
@dobertRowneySr dobertRowneySr force-pushed the giza-olympia_fix_BundledVideoCreationAndNFTIssuance branch from cfd1513 to a6d433f Compare February 4, 2022 12:44
Copy link
Contributor

@shamil-gadelshin shamil-gadelshin left a comment

Choose a reason for hiding this comment

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

LGTM

@shamil-gadelshin shamil-gadelshin merged commit bf0d7c7 into Joystream:olympia Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants