Grant-Milestone-Delivery icon indicating copy to clipboard operation
Grant-Milestone-Delivery copied to clipboard

add: substrate_core_polywrapper-milestone_1.md

Open boorich opened this issue 2 years ago • 8 comments

Milestone Delivery Checklist

Link to the application pull request: https://github.com/w3f/Grants-Program/pull/676

boorich avatar Oct 24 '22 08:10 boorich

Thank you for the delivery @empea-careercriminal. Could @mpetrunic confirm that you're authorised to make the delivery, since he originally submitted the application? And if you want to receive payment to a different address, could you please submit an amendment and modify it in the application file? Ideally, @mpetrunic would also sign off on that.

alxs avatar Oct 26 '22 13:10 alxs

Thank you for the delivery @empea-careercriminal. Could @mpetrunic confirm that you're authorised to make the delivery, since he originally submitted the application? And if you want to receive payment to a different address, could you please submit an amendment and modify it in the application file? Ideally, @mpetrunic would also sign off on that.

Confirming :)

mpetrunic avatar Oct 26 '22 13:10 mpetrunic

@alxs the address for payment is the one indicated in the invoice that we uploaded. Where can I check if this matches you records or if we need to file an amendment for this?

boorich avatar Oct 27 '22 07:10 boorich

@empea-careercriminal you provided one in your application.

alxs avatar Oct 27 '22 08:10 alxs

The address indicated in the application is fine. Please use that.

boorich avatar Oct 28 '22 11:10 boorich

Hey @empea-careercriminal, could you please take a moment to go through the original application and either submit an amendment such that the described functionality matches that of the current delivery, or make changes to the delivery? For example, deployment to IPFS, some functionality for substrate-signer-provider looks like it hasn't been implemented, the API for substrate-core-wrapper looks different, etc. For implementation changes with no difference in scope such as possibly the last one, you can instead provide a summary of changes. If there are any other differences, please include them in the amendment/delivery now as that will make the rest of the evaluation process easier for both of us.

alxs avatar Oct 31 '22 17:10 alxs

Hi @alxs. I'm one of the engineers who worked on the integration so I'll respond on behalf of @empea-careercriminal.

  1. IPFS Deployment - The wrapper is not ready for deployment to IPFS (as listed in the M1 submission). I'm also not sure if this is something that should be handled by the devs and might be better as a CD task in the polywrap repo. I will amend the original application to omit this task

  2. signer-provider plugin - This component is complete and exposes all signing functionality provided by the polkadot-js browser extension. I believe the original proposal was flawed and included too much scope for this component. As it is implemented it only handles the signing interaction with the browser plugin which is a much cleaner seperation of concerns.

  3. rpc-wrapper API - The list of functions givn in the proposal appears to be a list of Substrate RPC methods. This is a bit misleading and I'm not sure why it was written this way. The API we have chosen does provide all of the functionality of these RPC methods and some additional helpers. The proposal has been updated to include the actual interface instead.

I've submitted an amendment to the application to help clarify and removed the IPFS submission requirement. Please see the PR in w3f/Grants-Program#1262

Thanks and I hope this clears thing up.

willemolding avatar Nov 07 '22 18:11 willemolding

@willemolding thank you for the answer and submitting the amendment. Could you please update the delivery file and/or the amendment such that the lists of deliverables match? Note that deliverables 0a-d. are mandatory and need to be present in both. Most of these seem to be missing from the delivery, for instance tutorial, testing guide and article. Please also have a look at our milestone delivery guidelines.

Besides, there seem to be a number of unresolved issues with the Substrate core wrapper. Is this information outdated?

alxs avatar Nov 10 '22 13:11 alxs

@alxs thank you for your feedbacks. We are working on those with our engineering and the Polywrap teams. Regarding the unresolved issue, this information are outdated indeed.

Redoudou avatar Nov 14 '22 23:11 Redoudou

@alxs We updated the codebase and here's the corresponding article. Would you take a look at both of this?

I am aware of that pending CLA agreement and waiting for this to be resolved soon.

boorich avatar Nov 25 '22 11:11 boorich

On hold pending approval of the amendment.

Thank you both! I can't provide feedback on the article myself, but if you want us to proofread it and/or retweet an announcement about it, you can send it to [email protected].

Regarding the codebase, I'll go over the delivery again as soon as the amendment has been accepted. A reminder to also update the delivery file.

alxs avatar Nov 25 '22 16:11 alxs

@alxs Just checking if you have got all you need from our side to evaluate the amendment? Please ping in case something is blocking.

boorich avatar Dec 01 '22 09:12 boorich

Thanks @empea-careercriminal, see my comment in the amendment PR. The list of deliverables in the delivery file and the application need to match (you may want to update the delivery file instead). Feel free to argue against the reduction in price.

alxs avatar Dec 01 '22 13:12 alxs

@alxs Just checking in to ask if this is good now, or if you require other changes, so this can be approved?

boorich avatar Dec 16 '22 08:12 boorich

@empea-careercriminal thanks for following up. I would also like to test its usage, which I tried to do as shown in the example here. If I'm not mistaken, for now substrate-signer-provider-plugin-js and @polywrap/substrate-rpc-wrapper need to be local dependencies, and the wrapper needs to be built before being used. However, I tried following the instructions here and got the following errors with yarn:

...
error An unexpected error occurred: "https://registry.yarnpkg.com/substrate-signer-provider-plugin-js: Not found".
info If you think this is a bug, please open a bug report with the information provided in "/Users/aleixo/repos/tmp/integrations/protocol/substrate/rpc-wrapper/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

and yarn codegen:

yarn run v1.22.15
$ npx polywrap codegen
🔄 Manifest loaded from ./polywrap.yaml
✅ Manifest loaded from ./polywrap.yaml
🔄 Generate types
❌ Failed to generate types: Error: ABI not found at /Users/aleixo/repos/tmp/integrations/protocol/substrate/node_modules/@polywrap/http-interface/build/wrap.info
Error: ABI not found at /Users/aleixo/repos/tmp/integrations/protocol/substrate/node_modules/@polywrap/http-interface/build/wrap.info
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Do you have a working example somewhere, or could you guide me through this?

alxs avatar Dec 16 '22 16:12 alxs

@dOrgJelli @willemolding Would you mind providing some guidance?

boorich avatar Dec 19 '22 08:12 boorich

Hey apologies for the wait, just seeing this now @alxs @empea-careercriminal.

So the plugin is not currently published as it's in a pre-release state ATM, so yes in order to build the wrapper you need to have the plugin locally. We explain this here: https://github.com/polywrap/integrations/tree/main/protocol/substrate#developer-setup

If you'd like to verify the wrapper & plugin both build locally, you can follow these steps:

git clone https://github.com/polywrap/integrations
cd integrations/protocol/substrate
nvm use
yarn
cd ./signer-provider-js
yarn build
cd ../rpc-wrapper
yarn build

It might be useful to see the CI steps that are run, which show this, and how to run tests successfully: https://github.com/polywrap/integrations/blob/d5cdebe13c33ae9623c4e7726c720c579b458c56/.github/workflows/substrate-ci.yaml#L27-L37

To avoid this entirely, I've published a pre-release version of the plugin to NPM here: https://www.npmjs.com/package/substrate-signer-provider-plugin-js

I've created a demo application to show the wrapper working in an end user application here: https://github.com/dOrgJelli/substrate-wrapper-test

You can run this example by simply running:

git clone https://github.com/dorgjelli/substrate-wrapper-test
cd substrate-wrapper-test
nvm use
yarn
yarn build
yarn test

Which should output:

...local node setup (docker output)...
****************************************
Block Hash:  0x1ece48cf1ebfadae2a8a40b2c2f3c580f51f2babdf1523a80aa5e1c050411cfe
****************************************
...local node tear down (docker output)...

And the code which emits this can be found here: https://github.com/dOrgJelli/substrate-wrapper-test/blob/fb9d075768d9e2fecbf97f0f3b37cda3938f5640/src/index.ts#L8-L17

Happy to answer any further questions.

dOrgJelli avatar Dec 21 '22 17:12 dOrgJelli

Thanks a lot for the instructions and for setting up the example project. That was very helpful.

Regarding testing, it would be great to have tests for cases other than the happy path and those checking that a call doesn't return any errors. The one that was commented out is also still being skipped, which I think just needs to be fixed?

Lastly, the rpc-wrapper tests fail to run for me with following output: yarn test.txt.

Note that @keeganquigley will be taking over this evaluation in the new year. I believe the milestone can be accepted once the above has been addressed. My evaluation notes can be found here.

alxs avatar Dec 29 '22 17:12 alxs

Thank you @alxs for the review. With regards to the remaining issues, may I ask for your feedback @dOrgJelli?

boorich avatar Jan 03 '23 07:01 boorich

Hi @empea-careercriminal @dOrgJelli thanks for the changes. I am able to get all tests to run including rpc-wrapper. You can see my final notes here. That leaves the only remaining issue to be the skipped test that Alex mentioned above. Is there any way this test can be fixed? Also feel free to add more paths, as mentioned in the comment above. Thanks!

keeganquigley avatar Jan 12 '23 17:01 keeganquigley

Hello @keeganquigley , I work with @empea-careercriminal and we're looking into it. I'll have more for you soon!

stonecharioteer avatar Jan 19 '23 10:01 stonecharioteer

Hi @stonecharioteer are you able to give any updates?

keeganquigley avatar Feb 03 '23 16:02 keeganquigley

Hi @keeganquigley, yes, we were working on this. Here's the PR

  • Refactored the implementation of extrinsic API
  • Fixed the JsonRPC API of author_submit_extrinsic
  • Use sr25519 keypair in e2e tests

Note that this will have some breaking changes, since the extrinsic APIs in rpc-wrapper are modified.

stonecharioteer avatar Feb 06 '23 09:02 stonecharioteer

thanks @stonecharioteer much appreciated. I see the PR is merged, does this mean I can continue re-evaluating the milestone or are we still waiting on more changes?

keeganquigley avatar Feb 06 '23 15:02 keeganquigley

Yes @keeganquigley, you should be good to go. The failing test that was halting evaluation is now functional, and running in CI successfully. You can see a run of this here: https://github.com/polywrap/integrations/actions/runs/4094209269/jobs/7060221782

Or just test locally, steps should not have changed:

git clone https://github.com/polywrap/integrations
cd integrations/protocol/substrate
nvm use
yarn
cd ./signer-provider-js
yarn build
yarn test
cd ../rpc-wrapper
yarn build
yarn test

Please let us know if you have any issues! :)

dOrgJelli avatar Feb 07 '23 00:02 dOrgJelli

thanks for the changes @dOrgJelli much appreciated. Everything works now! I'm happy to accept this milestone, and you can find my final evaluation notes here.

Cheers and looking forward to the next one.

keeganquigley avatar Feb 10 '23 23:02 keeganquigley

@empea-careercriminal @dOrgJelli @stonecharioteer apologies for not realizing this earlier, but your invoice doesn't include the VAT ID on it, which is required for payment. Could you please submit an updated invoice with the W3F VAT ID included? You can find this by clicking through the invoice form.

Thank you!

keeganquigley avatar Feb 10 '23 23:02 keeganquigley

We noticed that this is the last milestone of your project, so what are the potential next steps?
While you can always apply for a follow-up grant, the grants program might not be the best tool for your future growth. Our main goal is to support research as well as early-stage technical projects. Below are a couple of other options in our ecosystem that might be better suited as the next step, depending on your goals:
Project with a Business Case/Token: Substrate Builders Program or VC Funding
Common Good Projects: Treasury Funding
Let us know if you have any questions regarding the above. We are more than happy to point you to additional resources and help you determine the best course of action.

github-actions[bot] avatar Feb 11 '23 22:02 github-actions[bot]

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we’ve created a badge for projects that successfully deliver their first milestone. Please use the badge only in reference to the work that has been completed as part of this grant, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant. Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation’s guidelines in doing so. If you haven't already, reach out to [email protected] for feedback on your announcement and cross-promotion.

Thank you for your contribution, and good luck with the remaining milestones, if any! As usual, let us know if you encounter any delays during your remaining milestones, by leaving a comment on the application PR or submitting an amendment.

github-actions[bot] avatar Feb 11 '23 22:02 github-actions[bot]

Thank you for the updated invoice! It has been forwarded internally for payment.

keeganquigley avatar Feb 13 '23 16:02 keeganquigley