Skip to content

Faucet milestone delivery #1#573

Merged
Noc2 merged 1 commit intow3f:masterfrom
karooolis:master
Oct 13, 2022
Merged

Faucet milestone delivery #1#573
Noc2 merged 1 commit intow3f:masterfrom
karooolis:master

Conversation

@karooolis
Copy link
Copy Markdown
Contributor

Milestone Delivery Checklist

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

@karooolis karooolis changed the title Faucet milestone delivery Faucet milestone delivery #1 Sep 22, 2022
@karooolis karooolis force-pushed the master branch 2 times, most recently from a147252 to 6f27a8c Compare September 22, 2022 15:25
@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Sep 28, 2022

Hi @karooolis! Thanks for the deliveries, and sorry for the late response!

I'm beginning the evaluation (as an external evaluator): I managed to run the application locally, and the .env configuration process is a little challenging. Could you please provide a minimum of help inside the docs to set everything up locally?

For example, I created a Twitter API app and GitHub API app, but just the GitHub authentication works, so I must miss something for the Twitter auth; also, I imagine the mnemonic must be of a non-empty wallet, could you explain please also how to prepare that address (generation + fill with test tokens) inside the Substrate environment?

A guide for the .env configuration would simplify the evaluation (e2e tests have to be run), but most importantly the integration for other projects.

Lastly, I cannot authenticate inside the demo you provided (either Twitter or GitHub).

I will start to write the evaluation document ASAP, so I can include these and more comments (you can already work on the mentioned things)

@karooolis
Copy link
Copy Markdown
Contributor Author

karooolis commented Sep 28, 2022

Hi @0xCaso, thanks for the clear initial feedback! You may now try to login with GitHub, will address Twitter auth and other points the following day. Talking about the wallet, indeed it must be non-empty. However, I'm not sure about the generation + filling with test tokens part. I'd say it's up to the faucet owner to fill the wallet and obtain the tokens for the faucet, so that's outside of the project scope.

@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Sep 28, 2022

@karooolis Thanks for the answer!

For the local configuration, how did you set up your test faucet? Are you using Substrate? Because I could reproduce your setup in order to test everything, to make things easier.

@karooolis
Copy link
Copy Markdown
Contributor Author

karooolis commented Sep 30, 2022

@0xCaso I improved the docs for configuration variables, and also local node setup that can be used for testing. Also, there was auth providers configuration mismatch, fixed it now. Please let me know what you think and open for further feedback!

@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Oct 1, 2022

Hi @karooolis! I created the evaluation document, you can see it here :)
As you can see, I requested some little changes, but overall your submission is almost perfect.

@keeganquigley keeganquigley self-assigned this Oct 3, 2022
@dsm-w3f dsm-w3f self-assigned this Oct 3, 2022
@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 3, 2022

@karooolis thank you for the milestone submission and my apologies for the late response. Please see the evaluation document and provide proper answers and fixes. After that, let me know to continue with the evaluation.

@keeganquigley keeganquigley removed their assignment Oct 3, 2022
@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Oct 3, 2022

Hi @dsm-w3f, should I close my PR?

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Oct 4, 2022

Hi @dsm-w3f, should I close my PR?

Thanks for your external evaluation. Please create PR to merge your evaluation. It’s actually better to have multiple evaluations of the same delivery.

@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Oct 4, 2022

Thanks for your external evaluation. Please create PR to merge your evaluation. It’s actually better to have multiple evaluations of the same delivery.

Thank you David, that makes sense! It's already opened: #585

@karooolis
Copy link
Copy Markdown
Contributor Author

karooolis commented Oct 5, 2022

Hi @dsm-w3f, no worries and thanks for the detailed feedback! I have made the changes, specifically:

  • Provided a fully configured .env.sample file that works out-of-the-box.
  • Improved Twitter setup description, and added link to official Twitter docs on how to obtain Twitter OAuth1.0 credentials.
  • Added warning that all env variables are mandatory to run the system.
  • Added JSDocs to all functions & classes.
  • Added comprehensive test suite.
  • Now take into consideration if wallet address has been used for claiming tokens.
  • Now also use user's email to check if user has already claimed funds.
  • Fixed ESLint errors & added pre-commit hook for code style, formatting and type checks.
  • Fixed GitHub authentication refresh issue.
  • Improved documentation to include npm instructions.
  • Locked installation to only be available with compatible node and npm versions.

@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Oct 6, 2022

Hi @karooolis, thank you for the changes! I've updated my evaluation document here.

@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 7, 2022

@karooolis thank you for the improvements. The evaluation is almost done, just one automated test case failing. The complete report is in the evaluation document.

@karooolis
Copy link
Copy Markdown
Contributor Author

Thanks @0xCaso and @dsm-w3f for feedback. The problem was related to authentication problem while doing E2E testing, got it fixed!

@0xCaso 0xCaso mentioned this pull request Oct 11, 2022
@0xCaso
Copy link
Copy Markdown
Contributor

0xCaso commented Oct 11, 2022

Thanks for the fix @karooolis!
Everything is perfect now, I updated the evaluation here as "Accepted", let's wait now for @dsm-w3f approval.

@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 13, 2022

@karooolis The e2e test is still failing for me. I tried to debug and the JWT generated for authentication is invalid. Maybe the fields from the fixture and JWTPayload type are not matching. The JWT generated for me is in the code block below, you can inspect it using https://jwt.io/. Let me know if I need something to bypass this problem since it seems to be working for you and Mateo.

eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIn0..t0TiOnbb5tNgOYJ9.no79Fa2PdSBjzks9dN8kO_IO8sgtzbLjQs_W7TjpxX-A4xHY_HOxrAxJkQ2voZBZVxEiJ-Ly6qkD_RgKuxyQuobgQpLEMw-vER4lz6G6VwNGRsJ7FIEwUxstx-Ht_8Fgp_XdUawP-n3wNBSFTDtVbw-uehUXQCGAnAwbPljLD7A3leo6l0gWEy-VAYDQSeIUsmaWR684Dd2mM0MMzmVjsAGHsw7ErAea_5szkYfSnNHOaK__8QjHo8CBIx7vqkh4_k4.Ibyu4oSbpyM8BSOcvoDQjQ

@karooolis
Copy link
Copy Markdown
Contributor Author

karooolis commented Oct 13, 2022

@dsm-w3f Sincere apologies, getting around auth seems to be a little capricious. Turns out, the cookie expired on October 12th, my oversight. I set the expiration date to 2140, now all seems to look good 🤞

@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 13, 2022

@karooolis thank you for the changes. The tests are now passing and the evaluation is approved. Thank you @0xCaso for the external evaluation in this delivery. I'll handle the internal procedures to pay you both.

@Noc2 Noc2 merged commit 5f6fcbf into w3f:master Oct 13, 2022
@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 13, 2022

@karooolis the Ethereum addresses from the contract and invoice are different. For safety reasons, they should always match. You will need to update the contract or the invoice.

@karooolis
Copy link
Copy Markdown
Contributor Author

@dsm-w3f Got it, just resubmitted the invoice 👌

@dsm-w3f
Copy link
Copy Markdown
Contributor

dsm-w3f commented Oct 13, 2022

@karooolis your invoice was forwarded internally and the payment should take place within two weeks (could happen before). @0xCaso your payment information also was forwarded and should take place with the same deadline (also could happen before that). Thank you guys, great work!

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.

5 participants