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.

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

Make local dependency version of orml_nft #32

Conversation

justinfrevert
Copy link
Contributor

@justinfrevert justinfrevert commented Jun 22, 2021

This copies the orml_nft source code and brings it into a local pallet, anmol-nft. The goal of this is to bring the NFT logic to a place where we can modify it and add logic for e.g. fractionalization.

All of the code in pallets/anmol-nft is unchanged from the 0.4.0 version of orml-nft

I still had some questions on this, which I will put below, but I think that this PR at least fulfills what was discussed here. A direct follow-on task is to add the logic described here in 2.

@justinfrevert
Copy link
Contributor Author

justinfrevert commented Jun 22, 2021

I had questions on quality process:

  1. Are we running cargo fmt on the code with this config? I noticed a lot of diffs when I did this, so I thought that either we're not doing this or some other code linting process is followed(or this is intended and we just need to run the command). General guidance here is appreciated.
  2. Is there a good way to validate the changes? In this case, I expect a no-op since the code isn't actually changing. But in most cases, I would like a way to see and test changes manually.

@korzewski
Copy link
Contributor

I had questions on quality process:

  1. Are we running cargo fmt on the code with this config? I noticed a lot of diffs when I did this, so I thought that either we're not doing this or some other code linting process is followed(or this is intended and we just need to run the command). General guidance here is appreciated.
  2. Is there a good way to validate the changes? In this case, I expect a no-op since the code isn't actually changing. But in most cases, I would like a way to see and test changes manually.
  1. We're not using cargo fmt yet. Haven't trying it out before, if you see some benefits of using it feel free to create a new ticket with concise description here https://www.notion.so/1b047c821544433d8703f352d347b207?v=7a4133f9bd00425d8f6f7292ad2fe3a3. Is it something like https://prettier.io/ for web?

  2. I think simple cargo check & cargo test is enough in this case. Both of them are already triggered as Github actions with a positive result ;)


I think this feature would be merged to ibtida as a second milestone. My suggestion would be to have a parallel feature branch eg. feature/nft-fractionalization, @nblogist please confirm.

Apart of that, once @nblogist accept, feel free to merge. All looks good to me!

@korzewski korzewski changed the base branch from ibtida to feature/nft-fractionalization June 23, 2021 06:34
@korzewski korzewski changed the base branch from feature/nft-fractionalization to ibtida June 23, 2021 06:40
@korzewski korzewski changed the base branch from ibtida to feature/nft-fractionalization June 23, 2021 06:40
@justinfrevert
Copy link
Contributor Author

@korzewski

Is it something like https://prettier.io/ for web?

I've used prettier, and looking at this cargo fmt tool it seems similar enough.

I can create a ticket, I'm not sure if it would require any research, but playing around with the tool so far it's been easy enough to find a config that mostly follows our style guide.

@aelesbao
Copy link
Contributor

@justinfrevert about rust fmt, I configured it with the coding guidelines at the beginning of the project but never applied it to the code forked from the template. I think it would be worth fixing the code in a separate PR and run it as a linting step during the build. I'll create an issue for that.

aelesbao
aelesbao previously approved these changes Jun 23, 2021
Copy link
Contributor

@aelesbao aelesbao left a comment

Choose a reason for hiding this comment

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

LGTM otherwise!

pallets/anmol-nft/Cargo.toml Outdated Show resolved Hide resolved
@nblogist
Copy link
Member

I think this feature would be merged to ibtida as a second milestone. My suggestion would be to have a parallel feature branch eg. feature/nft-fractionalization, @nblogist please confirm.

Agreed! @justinfrevert please rename the base branch to match the coding guidelines and branch it out from ibtida branch once #34 is merged.

Copy link
Member

@nblogist nblogist left a comment

Choose a reason for hiding this comment

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

Please go through my comments @justinfrevert

@justinfrevert justinfrevert changed the base branch from feature/nft-fractionalization to ibtida June 25, 2021 02:06
@justinfrevert justinfrevert dismissed aelesbao’s stale review June 25, 2021 02:06

The base branch was changed.

@justinfrevert
Copy link
Contributor Author

@nblogist I updated with the more descriptive pallet name, the updated description, etc. Can you let me know if the changes look okay, as well as the commit/branch naming?

FYI when going through the changes, renaming the pallet caused a lot of file diffs, as expected.

@nblogist
Copy link
Member

@justinfrevert base branch would be something like: frevert/feature/nft-fractionalization

@justinfrevert justinfrevert deleted the frevert/feature/copy-nft-pallet branch June 25, 2021 11:24
@nblogist nblogist restored the frevert/feature/copy-nft-pallet branch June 25, 2021 11:25
@justinfrevert justinfrevert deleted the frevert/feature/copy-nft-pallet branch June 25, 2021 11:30
@justinfrevert justinfrevert restored the frevert/feature/copy-nft-pallet branch June 25, 2021 11:32
@justinfrevert justinfrevert reopened this Jun 25, 2021
@justinfrevert justinfrevert changed the base branch from ibtida to frevert/feature/nft-fractionalization June 25, 2021 11:33
Copy link
Member

@nblogist nblogist left a comment

Choose a reason for hiding this comment

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

Looks good to me

@nblogist nblogist requested a review from aelesbao June 25, 2021 12:00
@justinfrevert justinfrevert merged commit fe24696 into frevert/feature/nft-fractionalization Jun 25, 2021
@justinfrevert justinfrevert deleted the frevert/feature/copy-nft-pallet branch June 25, 2021 12:26
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.

None yet

4 participants