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

Substratesnap_Maintenance-3 #549

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

danforbes
Copy link
Contributor

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#874

Copy link
Contributor

@takahser takahser left a comment

Choose a reason for hiding this comment

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

@danforbes thanks for your delivery. Which month(s) does this delivery cover? I saw that you've uploaded 2 bills:

Also, it would be nice if you could add some more details to your delivery (e.g. "feat CI CD" doesn't give me a lot of information, other than that it's probably something CI/CD related).

@takahser takahser requested a review from Noc2 September 14, 2022 13:27
@danforbes
Copy link
Contributor Author

Thank you for your notes, @takahser. I have updated the delivery document with some additional details. The snap has been released to NPM and should be available for people in the community to use. There is also a PR that has been open for over two weeks to integrate snap support into Polkadot-JS Apps UI. Please let me know if there are additional details that I can provide. I have a meeting w/ the grants team on Monday and I'd be happy to demo the snap at that time or any other that may be better for you and colleagues.

Copy link
Contributor

@takahser takahser left a comment

Choose a reason for hiding this comment

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

I've a couple of points to make here.

  1. Quote from your wiki:

    Metamask Polkadot snap is enabled through metamask-polkadot-adapter.

    I think this is outdated. @nodefactory/metamask-polkadot-adapter has been deprecated in favor of @chainsafe/metamask-polkadot-adapter. That's also the package that you've used for the integration into polkadot-js apps.

  2. It would be nice if you could add the README to the npm package:

    image
  3. Also, feel free to have a look at my comments on the polkadot-js/apps integration.

Copy link
Collaborator

@Noc2 Noc2 left a comment

Choose a reason for hiding this comment

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

@danforbes I’m going to merge this PR and forward the invoice internally. But you might still want to take a look at @takahser comments.

@Noc2 Noc2 merged commit e2a3cc1 into w3f:master Oct 17, 2022
@danforbes danforbes deleted the dforbes/Substratesnap_Maintenance-3 branch October 21, 2022 14:46
@danforbes
Copy link
Contributor Author

Apologies for the delay - the snap has been released and the README now appears on the NPM page https://www.npmjs.com/package/@chainsafe/polkadot-snap. The requested changes to the wiki have also been made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants