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.

Sign up for GitHub

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

Jump to bottom

feat: add support for foreignAssets pallet #362

Merged
merged 2 commits into from
Mar 4, 2024
Merged

feat: add support for foreignAssets pallet #362

merged 2 commits into from
Mar 4, 2024

Conversation

bee344
Copy link
Collaborator

@bee344 bee344 commented Mar 4, 2024

Added support for the foreignAssets instance of the assets pallet. Also fixed the tests that used the kusama asset hub and previously referenced polkadot asset hub, added the PAH metadata and registry, and added tests for the foreignAssets pallet.

Closes #361

@bee344 bee344 requested a review from a team as a code owner March 4, 2024 16:12
TarikGul
TarikGul approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Some notes:

The more metadata we add to the tests in txwrapper-dev the higher the chance the CI is going to timeout and fail the tests... It's still an unresolved issue we don't understand. But just wanted to put that as a reminder for us.

Imod7
Imod7 reviewed Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

Docs update ?

@TarikGul
Copy link
Member

TarikGul commented Mar 4, 2024 •
edited
Loading

Docs update ?

We only do doc updates before a release now since it updates 400 files.

lexnv
lexnv reviewed Mar 4, 2024
View reviewed changes
packages/txwrapper-dev/src/registries/assetHubPolkadotRegistry.ts Outdated
Comment on lines 19 to 21
ss58Format: 2,
tokenDecimals: 10,
tokenSymbol: 'DOT',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check, shouldn't the prefix version of the AssetHub be zero? Similar to Polkadot?

Similar to ss58-registry and Asset Hub chain Spec? The prefix == 2 is dedicated to kusama

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, thanks for catching that!

@bee344
Copy link
Collaborator Author

bee344 commented Mar 4, 2024

Docs update ?

Not yet, we are going to do the docs before the releases, as to not clutter the PRs

@Imod7
Copy link
Collaborator

Imod7 commented Mar 4, 2024 •
edited
Loading

Docs update ?

Not yet, we are going to do the docs before the releases, as to not clutter the PRs

Maybe have this reflected somewhere ? In the release process (if its not there already) ?

@bee344
Copy link
Collaborator Author

bee344 commented Mar 4, 2024

Docs update ?

Not yet, we are going to do the docs before the releases, as to not clutter the PRs

Maybe have this reflected somewhere ? In the release process (if its not there already) ?

Done #363

lexnv
lexnv approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Looks clean to me (with my limited knowledge of ts), nice job here 👍

@bee344 bee344 merged commit 2fa941e into main Mar 4, 2024
6 checks passed
@bee344 bee344 deleted the anp-foreign-assets branch March 4, 2024 17:07
@bee344 bee344 mentioned this pull request Mar 18, 2024
Merged
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/parity-tech-update-for-march/7226/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lexnv lexnv lexnv approved these changes

@TarikGul TarikGul TarikGul approved these changes

@Imod7 Imod7 Imod7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for foreign-assets
5 participants
@bee344 @TarikGul @Imod7 @Polkadot-Forum @lexnv