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

chore: use cdn hosted assets registry #327

Closed
wants to merge 5 commits into from

Conversation

marshacb
Copy link
Contributor

  • removes the @substrate/asset-transfer-api-registry npm package
  • constructs the ATA registry by fetching the latest registry data from https://paritytech.github.io/asset-transfer-api-registry/registry.json

@TarikGul
Copy link
Member

TarikGul commented Nov 20, 2023

One concern I have is I see the registry keeps updating every hour and is deleting and re-adding a few parachains.

I am looking into this, but this shouldn't go in until that is resolved.

@TarikGul
Copy link
Member

We also need a way for users to opt out of the cdn registry and opt in for the static npm registry

@TarikGul
Copy link
Member

I would also change the commit name, since this is a fundamental change to the API this is going to break things. It's more of a fix!.

On the topic of fundamental breaking changes. We can avoid breaking the API all together by initializing the registry once the first call is made. Basically the first time createTransferTransaction is called we see if the registry has been initialized, if it hasn't we set it, and if it has we do nothing.

That way the API stays super simple for the user, and we don't apply any breaking 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.

Overall, I think we should avoid adding the registry as a constructor input since it adds complexity for the user, and we should maintain handling everything internally. (Suggestion above)

There should be an opt in, and opt out option that allows the user to choose where the registry comes from as well, whether its fetched from the CDN or pulled in from the npm package.

Addition: I know in private conversation I agreed to the current implementation, but after some review I think we should be careful adding any breaking changes to the core public facing functionality and DX unless it is absolutely necessary or we introducing a whole new API in general.

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.

2 participants