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

Feat: add Heiko kbtc #50

Merged
merged 10 commits into from
Mar 1, 2023
Merged

Feat: add Heiko kbtc #50

merged 10 commits into from
Mar 1, 2023

Conversation

bvotteler
Copy link

@bvotteler bvotteler commented Feb 16, 2023

Resolves #43

Add XCM support for Heiko: KBTC

Links to constructed test tx:

@bvotteler bvotteler marked this pull request as ready for review February 16, 2023 12:13
@bvotteler bvotteler requested a review from tomjeatt February 16, 2023 12:14
@sander2
Copy link
Member

sander2 commented Feb 16, 2023

Since we will have a bunch of these prs, I would propose to setup some PR guidelines. Specifically, before merging such prs, I'd like to see some kind of proof that generated extrinsic actually works. This could be:

  • If the channel and both chains are set up, then:
    • if there are recent transactions already, provide a subscan link to a historic successful transfer, plus the polkadotjs link of the generated encoded extrinsic. Reviewers can compare the both.
    • if no transactions were made yet, use the code to make a test tx yourself and provide the subscan link to that tx here. Reviewers can verify that it works
  • If the channel or one of the chains is not ready yet, provide only a polkadotjs link of the generated encoded extrinsic. Reviewers can verify that the output looks plausible

@bvotteler
Copy link
Author

bvotteler commented Feb 16, 2023

Since we will have a bunch of these prs, I would propose to setup some PR guidelines. Specifically, before merging such prs, I'd like to see some kind of proof that generated extrinsic actually works. This could be...

Good suggestion, thank you.

For these PRs, I will not have the means to test the transactions (no funds to shuttle across the chains repeatedly). I'm not interested in gaining access to Interlay's test accounts, either. It's more secure the fewer people have access, plus there will be extensive tests once this is added to the UI.

It's easy enough to provide (links to) the generated extrinsics, so providing them is a no-brainer.
Would it make sense to search for and link similar xcm transactions (same token) to other chains for comparison if they exist?

@sander2
Copy link
Member

sander2 commented Feb 16, 2023

Since we will have a bunch of these prs, I would propose to setup some PR guidelines. Specifically, before merging such prs, I'd like to see some kind of proof that generated extrinsic actually works. This could be...

Good suggestion, thank you.

For these PRs, I will not have the means to test the transactions (no funds to shuttle across the chains repeatedly). I'm not interested in gaining access to Interlay's test accounts, either. It's more secure the fewer people have access, plus there will be extensive tests once this is added to the UI.

It's easy enough to provide (links to) the generated extrinsics, so providing them is a no-brainer. Would it make sense to search for and link similar xcm transactions (same token) to other chains for comparison if they exist?

It would help but that's not a guarantee it'll work due to differences in e.g. existential deposit & fees.

I think it's better to test as early as possible, rather than only after it gets added to UI. I get your point re: security, but if it's just a test account with < 100 usd worth of assets it's not much of a risk. I'd say it's worth it, especially since it also saves money in dev time if we catch problems as early as possible (no context switching, no coordinating of different people, no downstream redeployments, etc)

@bvotteler
Copy link
Author

bvotteler commented Feb 22, 2023

Local testing (chopsticks) - transferring KBTC
Succeeded with Alice account.

Kintsugi -> Heiko
Kintsugi:
Screen Shot 2023-02-22 at 16 04 12
Heiko:
Screen Shot 2023-02-22 at 16 04 25

Heiko -> Kintsugi
Heiko:
Screen Shot 2023-02-22 at 16 07 23
Kintsugi:
Screen Shot 2023-02-22 at 16 07 33

…oded extrinsics in local tests via chopsticks.
@bvotteler bvotteler requested a review from sander2 February 28, 2023 08:55
@bvotteler bvotteler requested a review from sander2 February 28, 2023 10:50
@sander2 sander2 merged commit 07a8ef0 into interlay:master Mar 1, 2023
@bvotteler bvotteler deleted the feat-add-heiko-kbtc branch March 1, 2023 16:23
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.

Support Parallel Heiko XCM
2 participants