llamaxyz / llama

Llama is an onchain governance and access control framework for smart contracts.
https://llama.xyz
MIT License
47 stars 5 forks source link

test: Extract Test setup to a common Module #38

Closed 0xrajath closed 1 year ago

0xrajath commented 1 year ago
mds1 commented 1 year ago

This would be great. I'd suggest doing this by pulling the "deployment of vertex instance" into Deploy.s.sol, writing tests in test/script/Deploy.s.sol, then running the deploy script as the shared test setup.

You can find more info about this here:

mds1 commented 1 year ago

Using high priority here just since this will affect a lot of tests, so I think it makes sense to get this in place before making other test changes

mds1 commented 1 year ago

As part of this let's consider updating the Makefile as discussed in https://github.com/llama-community/vertex-v1/pull/30#discussion_r1101866389:

Should consider a version of this that doesn't have the --broadcast flag so you can do a dry run without deploying (or, perhaps the intent is to do dry runs against anvil and pass that RPC URL in those cases, which is ok, though based on the current script I think you're just not done with deploy scrits yet)

mds1 commented 1 year ago

One other thing I want to call out is the "For scripts that read from JSON input files, put the input files in" section of the foundry Best Practices. I'd suggest following that format, where you'd have a chain ID of 31337 for local testing (i.e. the default chain ID in forge is 31337). Then you'd use the StdJson helpers to parse the input JSON