Deployment scripts #144
Deployment scripts #144
Conversation
| } | ||
| }); | ||
|
|
||
| await Promise.all(result); |
There was a problem hiding this comment.
Why not Promise.allSettled? This will break the script at the first error and discard successful work on other chains, perhaps entire successful deployments.
There was a problem hiding this comment.
cb is wrapped on a try/catch statement anyways, I don't think an error can bubble up, or could it? 🤔
There was a problem hiding this comment.
Hm, yeah, now that I look at it closer, I guess not. It would blow up when some other function fails like getSigner but that should be fine.
deployment/helpers/evm.ts
Outdated
| import { ethers } from "ethers"; | ||
| import { ChainInfo, ecosystemChains, getEnv, LoggerFn } from "./index"; | ||
|
|
||
| export type EvmScriptCb = (chain: ChainInfo, signer: ethers.Signer, logFn: LoggerFn) => Promise<void>; |
There was a problem hiding this comment.
This is practical for deployment and configuration scripts but you're enforcing read-only scripts to have a signer by implementing it this way. Which in the worst case means having a ledger connected and unlocked before running such a script 😛
There was a problem hiding this comment.
ye, not great right now... but at the same time truth is that I will always have the ledger connected when trying to run a script at the moment... so I decided not to worry about that as much rn, but added this as a requirement for our work methodology
There was a problem hiding this comment.
I think it could be worth writing an explicit constructorArgs object that is passed into the ethers arguments matching the ABI named parameters. This is supported for struct members but I don't remember if it's supported for function parameters 🤔 . In this way, we could automatically produce the list of arguments with a generic implementation whenever we need them for verification and also keep them human readable in the configuration.
deployment/helpers/env.ts
Outdated
| return dependency; | ||
| } | ||
|
|
||
| export function writeDeployedContract(chain: ChainId, contractName: string, address: string) { |
There was a problem hiding this comment.
I think we should start considering writing further output. For example, writing the tx hash that created a contract would be useful to implement specific kinds of deployments or verifying constructor arguments among other things.
There was a problem hiding this comment.
cool, sounds like a good idea. But do you agree that we don't need to worry as much about performance on this scripts so we can write atomic pieces of data to files instead of batching up multiple updates?
What I mean is that if we want to store some other datum we should create a simple interface like this one specifically to write the tx hash corresponding to 1 deployment
There was a problem hiding this comment.
Choosing a minimal interface would help reduce the burden on whoever writes the script though. I mean, if you figure out that just passing in the ethers contract object plus the receipt is good enough to write absolutely everything, then I see no reason to do any other thing. This is just an example, I'm not sure both of those suffice but they probably bring us very close to what the output should look like.
solanoepalacio
force-pushed
the
deployment-scripts
branch
from
a9a09f0
to
4b35045
Compare
|
Should this PR be converted to draft? It looks like the last commit mentions WIP. |
scnale
force-pushed
the
deployment-scripts
branch
from
1e60858
to
2457961
Compare
scnale
force-pushed
the
deployment-scripts
branch
from
db47f93
to
e761480
Compare
…om solana" This reverts commit 29fc5ad.
No description provided.