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

docs: move examples out of the workspace #144

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

isidroamv
Copy link
Collaborator

No description provided.

Copy link

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@zk-email/circuits 3.2.0 None +11 43.1 MB yushg
browserstack-node-sdk 1.31.2 shell +97 307 MB browserstack
@zk-email/zk-regex-circom 1.2.1 None +0 147 kB sora_suegami
@types/atob 2.1.4 None +0 2.66 kB types
ffjavascript 0.2.48 environment +4 39.5 MB jbaylina
jest-environment-jsdom 29.7.0 None +2 3.96 MB simenb
snarkjs 0.4.12 None +20 133 MB jbaylina
@types/styled-components 5.1.32 None +1 29.5 kB types
react-router-dom 6.19.0 environment +2 3.88 MB mjackson
@types/node-forge 1.3.10 None +1 3.98 MB types
@types/mocha 10.0.5 None +0 95.5 kB types
@types/jest 29.5.9 None +1 4 MB types
browserstack-local 1.5.5 environment +1 83 kB browserstack
@types/lodash 4.14.202 None +0 862 kB types
@types/node 20.9.3 None +0 3.92 MB types
wagmi 1.4.7 None +30 24.2 MB awkweb
vite-tsconfig-paths 4.2.1 None +2 4.24 MB aleclarson
big-integer 1.6.52 None +0 175 kB peterolson
chai 4.3.10 environment +0 752 kB keithamus
@testing-library/jest-dom 5.17.0 None +3 4.3 MB testing-library-bot
selenium-webdriver 4.15.0 environment +0 19.1 MB titusfortner
@rainbow-me/rainbowkit 1.3.0 None +35 28.7 MB danielsinclair
@vitejs/plugin-react 4.2.0 None +3 3.97 MB vitebot
@types/jest 29.5.10...29.5.2 None +2/-0 200 kB types

🚮 Removed packages: @rainbow-me/rainbowkit@1.0.9, @testing-library/jest-dom@5.16.5, @types/styled-components@5.1.26, @vitejs/plugin-react@4.0.0, big-integer@1.6.51, browserstack-local@1.5.3, browserstack-node-sdk@1.17.0, jest-environment-jsdom@29.5.0, react-router-dom@6.15.0, selenium-webdriver@4.10.0, serve@14.2.0, vite-tsconfig-paths@4.2.0, wagmi@1.3.10

@isidroamv
Copy link
Collaborator Author

@Divide-By-0 I believe run_forge_tests is failing because the env key ALCHEMY_API_KEY is not correctly setup. I see the same error in the previous PR. Can you check if the the ALCHEMY_API_KEY is okay?

@Divide-By-0
Copy link
Member

Divide-By-0 commented Nov 22, 2023

Can you just add this in plaintext as a backup if the alchemy api key is empty
: GOERLI_ALCHEMY_API_KEY 547924becf984d16b47dbbdaa0dc344d

@isidroamv

@isidroamv
Copy link
Collaborator Author

I have the same error with the key you provided:

Error: 
Could not instantiate forked environment with fork url: https://eth-goerli.g.alchemy.com/v2/547924becf984d16b47dbbdaa0dc344d

Context:
- Error #0: Failed to get latest block number
- Error #1: (code: -32600, message: Must be authenticated!, data: None)

Is the key working for you?

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 22, 2023

I have a key that works but it is a key that I created from my account in https://www.alchemy.com/dapps/goerli. The best practice is setup that key in the CircleCI Dashboard so we avoid putting API_KEY variables in the code

@Divide-By-0
Copy link
Member

Divide-By-0 commented Nov 23, 2023

I have a key that works but it is a key that I created from my account in https://www.alchemy.com/dapps/goerli. The best practice is setup that key in the CircleCI Dashboard so we avoid putting API_KEY variables in the code

We tried doing that, but external PRs don't get that env vars. I think let's just put your key in there for now.

You can also try: DvukuyBzEK-JyP6zp1NVeNVYLJCrzjp_

And: ALCHEMY_API_KEY=zcTJYSuLdudKRdf00wuKpz425B4x7qyr

Copy link

gitguardian bot commented Nov 23, 2023

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 23, 2023

The test cases are passing but it looks the security check didn't pass because the API_KEY is uncovered. It seems possible to configure env vars in the project settings and/or context under organization settings. If you give me access to the circleci project or organization I can figure out how to do it.

@Divide-By-0

@Divide-By-0
Copy link
Member

The test cases are passing but it looks the security check didn't pass because the API_KEY is uncovered. It seems possible to configure env vars in the project settings and/or context under organization settings. If you give me access to the circleci project or organization I can figure out how to do it.

@Divide-By-0

Sure, what messaging platform or email can i send your invite to?

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 23, 2023

The test cases are passing but it looks the security check didn't pass because the API_KEY is uncovered. It seems possible to configure env vars in the project settings and/or context under organization settings. If you give me access to the circleci project or organization I can figure out how to do it.
@Divide-By-0

Sure, what messaging platform or email can i send your invite to?

I have requested access or you can authorize my current github account.

@Divide-By-0
Copy link
Member

Divide-By-0 commented Nov 23, 2023

Hey, I've enabled using env vars on forked PRs. I recommend removing the secret, and it should now just work. @isidroamv

@isidroamv
Copy link
Collaborator Author

Tests are passing now.

Hey, I've enabled using env vars on forked PRs. I recommend removing the secret, and it should now just work. @isidroamv

@Divide-By-0
Copy link
Member

Divide-By-0 commented Nov 23, 2023

Awesome, finally! Send over an eth address so I can send you a small grant.

Can you pre-emptively rebase #143 and #146, so that they don't have to redo their PR in the new location of the moved files?

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 23, 2023

I can wait for them to merge their PRs into the main branch in case the have pending changes. So I can sync my branch with main with the latest changes.

@Divide-By-0
Copy link
Member

Can you move all of these changes to https://github.com/zkemail/proof-of-twitter/ and setup that CI? I think ultimately it's better for the examples to be seperate.

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 24, 2023

The only problem I see is that we use these examples as e2e test in this repo.

One alternative could be:

  1. Remove e2e testing from this repo and leaving only unit test
  2. Add a github action in this repo to trigger the e2e execution from the new repo using the current branch where it is executed.

If you go with this alternative would be good to remove the e2e from this repo util the new flow is configured.

@Divide-By-0
Copy link
Member

Divide-By-0 commented Nov 24, 2023

You're right; we need e2e testing here and in the twitter app?

Since that's the case, then I think we should make proof-of-twitter a gitmodules of this repo, so that new users can fork proof-of-twitter to start building their own apps, but also e2e tests here actually verify new functionality more robustly by checking it live against the twitter example. I've given you PR access on the proof-of-twitter repo.

@isidroamv
Copy link
Collaborator Author

isidroamv commented Nov 24, 2023

Currently, the twitter-app uses the real-time changes made in the packages and it is being used as an e2e test here.
I believe it is okay to move the examples to the proof-of-twitter repo as long we use the the correct branch in the e2e test.

So the only thing we would need in this repo is to modify the CI pipeline using the new location.

@saleel saleel changed the base branch from main to develop February 1, 2024 15:09
@saleel
Copy link
Member

saleel commented Feb 1, 2024

@isidroamv Thanks for the PR and the one in proof-of-twitter.
I will merge this develop and resolve the conflicts couple of related PRs. We can merge to main altogether. Since proof-of-twitter is ready now, I will also remove examples from here.
Tests seems to be fine now, but full e2e tests for twitter is not running in Github Actions now - There is issue created to handle this zkemail/proof-of-twitter#9.

@saleel saleel merged commit 3c0ad61 into zkemail:develop Feb 1, 2024
6 checks passed
@isidroamv
Copy link
Collaborator Author

Thanks @saleel. Just keep in mind that you also need to remove the following tests from the CI:

  • run_unit_and_e2e_tests

I think you may also want to remove these packages (with their respective test in the CI) since they also moved the twitter repository:

  • twitter-verifier-contracts
  • twitter-verifier-circuits

https://github.com/zkemail/zk-email-verify/blob/main/.circleci/config.yml

@saleel
Copy link
Member

saleel commented Feb 1, 2024

Yes, they will be removed. The tests here will be the unit tests for circuit, contract (DKIM registry) and some stuffs in helpers package.
All twitter tests will be in the other repo.

@saleel saleel mentioned this pull request Feb 7, 2024
9 tasks
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.

3 participants