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

Remove explicit private keys in the code and getSigner(0) from account creation in tests #402

Open
mariacarmina opened this issue Apr 24, 2024 · 6 comments · May be fixed by #833
Open

Remove explicit private keys in the code and getSigner(0) from account creation in tests #402

mariacarmina opened this issue Apr 24, 2024 · 6 comments · May be fixed by #833
Assignees

Comments

@mariacarmina
Copy link
Member

mariacarmina commented Apr 24, 2024

Use blockchain class signer instead of getSigner(0) -> does not work for other networks.

@paulo-ocean
Copy link
Contributor

paulo-ocean commented Apr 24, 2024

hi @mariacarmina , just some clarification. We don't necessarily need to generate other keys on all tests.. we could also have the getSigner(0) and getSigner(1) (or even more) addresses and hard code them on some constants. The important is to have some keys/addresses for testing... and avoid stuff like:

provider = new JsonRpcProvider('http://127.0.0.1:8545')
await provider.getSigner(0)) // this kind of code does not work outside ganache, will will give 'NO ACCOUNT' error

We could do like:

provider = new JsonRpcProvider(rpc)
signer/wallet = new ethers.Wallet(SOME_KEY, provider)

thanks

@mariacarmina
Copy link
Member Author

From what we discussed today in the sprint planning session, this adds a bit of workload, because we do not want to expose the private keys clearly through the code. We leave it as low prio.

cc @alexcos20

@paulo-ocean
Copy link
Contributor

paulo-ocean commented May 13, 2024

what do you mean by expose private keys?? We are explicitly setting the PRIVATE_KEY env var in 90% of the test suite!
its a matter of using with new ethers.Wallet(SOME_KEY, provider) (better; use the blockchain class we have)
this instead provider.getSigner(0) does not work anywhere outside ganache
I also disagree it generates a lot of workload... it would probably take 1 hour at most

take the first integration test as example (we are exposing a key):

ENVIRONMENT_VARIABLES.PRIVATE_KEY,
'0xc594c6e5def4bab63ac29eed19a134c130388f74f019bc74b8f4389df2837a58',
provider = new JsonRpcProvider('http://127.0.0.1:8545')
publisherAccount = (await provider.getSigner(0)) as Signer

to:

const chain = new Blockchain('http://127.0.0.1:8545','development',8996)
provider = chain.getProvider()
publisherAccount = chain.getSigner()

i do agree its low priority but i think we might have misunderstood the main point here

@mariacarmina
Copy link
Member Author

mariacarmina commented May 13, 2024

I also disagree it generates a lot of workload

I meant for setup of the tests, not actually doing the modifications

what do you mean by expose private keys?? We are explicitly setting the PRIVATE_KEY env var in 90% of the test suite!

I see that on 4 test files we use setupEnvironment with explicitly private keys PRIVATE_KEY and also NODE_PRIVATE_KEY => my approach is to remove where private keys are exposed in the tests and also use blockchain class forgetting signers. certainly not 90%

@paulo-ocean
Copy link
Contributor

paulo-ocean commented May 13, 2024

actually is 7 not 4 :-) (counting integration tests only) but my point is mainly about the getSigner(0) ... setup & modification is not hard... anyway, not priority for now, for sure

@mariacarmina
Copy link
Member Author

I was counting the unit tests which were 4, there are 11 in total in tests. I understood the point and the effort on this should be as well low, will tackle when higher prio tasks are solved.
Here are 2 points that need to be solved:

  • remove explicit PRIVATE KEYS from code
  • replace getSigner(0) with blockchain class signer

I'll change the title accordingly.

@mariacarmina mariacarmina changed the title Remove getSigner(0) from account creation in tests Remove explicit private keys in the code and getSigner(0) from account creation in tests May 13, 2024
@mariacarmina mariacarmina linked a pull request Feb 3, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants