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

docs: ADR-43: Aligning on interoperability #9709

Merged
merged 8 commits into from
Jul 26, 2021
Merged

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Jul 16, 2021

Description

Following on:

we need to be clear how the NFT token interoperability between modules and chains is achieved.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Jul 16, 2021
@robert-zaremba
Copy link
Collaborator Author

As discussed with @okwme - we may update a language in this ADR to highlight that x/nft is a base module to be used with other NFT implementations (rather than forked and updated). We can do it in this PR or in a new one.

@robert-zaremba robert-zaremba changed the title ADR-43: Aligning on interoperability docs: ADR-43: Aligning on interoperability Jul 16, 2021
@robert-zaremba
Copy link
Collaborator Author

cc: @iramiller , @MikeSofaer

@MikeSofaer
Copy link

I'm not sure I understand this. Normally I'd expect an interface spec, or some kind of duck typing around something like NFT IBC, not a requirement to implement using a particular piece of code. Shouldn't we be making sure that people who don't use x/nft know how to interoperate with it, rather than demanding everyone use it?

@robert-zaremba
Copy link
Collaborator Author

@Mikerah The interface is already defined by the protobuf definitions in the Msg Service section. The IBC standard is yet to be defined. What we require here, is that other implementations will use x/nft for the future IBC interoperability.

@MikeSofaer
Copy link

If what you're saying that x/nft will eventually support an IBC NFT standard, just say that.

@orijbot
Copy link

orijbot commented Jul 19, 2021

Visit https://dashboard.github.orijtech.com?pr=9709&repo=cosmos%2Fcosmos-sdk to see benchmark details.

@robert-zaremba robert-zaremba requested a review from aaronc July 26, 2021 14:43
@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 26, 2021
@mergify mergify bot merged commit c061cc1 into master Jul 26, 2021
@mergify mergify bot deleted the robert/nft-interoperability branch July 26, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants