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

Implement IBC profiles feature #412

Closed
wants to merge 142 commits into from
Closed

Implement IBC profiles feature #412

wants to merge 142 commits into from

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented May 4, 2021

Description

This PR implement #192, creating cross-chain link by IBC protocol and inter-chain link by message.

IBC profiles module features:

  • Create link struct on destination chain after relaying successfully
  • Support create-ibc-link method to create a link by one key
  • Provide create-ibc-connection method to create a link by two private keys used on target chain
  • Include integration test, which checks the final statement after sending ibc profiles transactions

IBC feature in profiles module:

  • Support cross-chain link creation service by receiving ibc-profiles packet from other chain
  • Offer inter-chain link creation service
  • Provide link storage in the database and Profile
  • Allow unlink action to delete the link

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@RiccardoM
Copy link
Contributor

@dadamu If you think it would be better, I guess we can use Bech32 without any problem. I used the hex representation to make it chain-independent, but if we provide the ChainConfig object then it should be the same to use the Bech32 format instead.

Also, I would avoid putting the SourceAddress field inside a Link object. Since links will be present inside a Profile, it would be redundant to put a SourceAddress inside each link. This value would be identical to the profile address, and so it can be easily removed.

@dadamu
Copy link
Contributor Author

dadamu commented May 17, 2021

@RiccardoM I got it, the cross-chain and inter-chain links are associated with Profile directly, not account. So desmos address is not needed in Link object. Thanks!

@dadamu dadamu requested a review from leobragaz May 21, 2021 06:34
@dadamu dadamu changed the title Implement IBC links module Implement IBC profiles feature May 21, 2021
@RiccardoM
Copy link
Contributor

RiccardoM commented May 24, 2021

Hey @dadamu, since this PR got quite big (more than 100 files to be checked), I think we should split it into multiple ones, each one with its review process.

To do this, what I would ask you to do is the following:

  1. Create a new branch named paul/ibc-links starting from the current master branch.
  2. Using that branch, create multiple consecutive PRs each one implementing one small change:
    1. The addition of Proto files
    2. The logic behind storing the links
    3. Tests
    4. Documentation

At each PR me and @bragaz will review it and once everything is done, merge it into the destination branch.

The overall process will be the following:

  1. Fork master and create paul/ibc-links.
  2. Put inside the paul/ibc-links the updated Proto files.
  3. Open a new PR and ask me and @bragaz the review.
  4. Once the review is approved and merged, add to the paul/ibc-links branch the logic to store the links.
  5. Open a new PR and ask me and @bragaz the review.
  6. Once the review is approved and merged, add to the paul/ibc-links branch the tests.
  7. Open a new PR and ask me and @bragaz the review.
  8. Once the review is approved and merged, add to the paul/ibc-links branch the documentation.
  9. Open a new PR and ask me and @bragaz the review.

Ideally, all the PRs should be made towards a new branch called paul/ibc-profiles that will have all the changes. Finally, we will create a paul/ibc-profiles -> master PR and merge everything when it's all clear and finalized.

This way we will be able to review all the things faster and give you a more rapid feedback.

If all of this is clear, please start with creating the branches and close this PR.

@dadamu dadamu closed this May 25, 2021
@dadamu dadamu deleted the paul/links branch June 8, 2021 09:53
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.

3 participants