New deployment result types #423
Conversation
| deploymentParameters: DeploymentParameters, | ||
| accounts: string[], | ||
| defaultSender: string | ||
| ): Promise<DeploymentResult> { | ||
| ): Promise<DeploymentResult<ContractNameT, IgnitionModuleResultsT>> { | ||
| await validateStageTwo( |
There was a problem hiding this comment.
This is something you can take over if you want @kanej.
Here the validate functions are throwing an error that doesn't give you access to the futureId that caused the error.
Ideally, the validation function would return all the errors. A quick fix is changing the error they throw so that they include the futureId. We can do the proper fix later.
There was a problem hiding this comment.
I am tackling this now.
I am going to alter the validate stage two to return [futureId, validationErrorMessage] pairs (or some such) that can then be returned as a result if there are any validation failures.
There was a problem hiding this comment.
Alright, I backed out of the larger change and did as you suggested.
Stage two now returns either a ValidationErrorDeploymentResult or null, and deploy returns the error result if detected.
There was a problem hiding this comment.
I have added an issue to expand out the number of validation messages returned: #424
| if (result.type === DeploymentResultType.EXECUTION_ERROR) { | ||
| console.log(result); | ||
| throw new IgnitionError( | ||
| `The deployment wan't succesful: TODO: create a good error message` |
There was a problem hiding this comment.
This is also something else that you can make progress on, @kanej
There was a problem hiding this comment.
What do you think of only showing the failed and timed out within the Ignition Error:
The deployment wan't succesful, there were timeouts and errors:
Timed out
* SomeContract
Errors
* MyContract: reverted with reason z
* AnotherContract: reverted with reason y
packages/hardhat-plugin/src/index.ts
Outdated
| verbose: logs, | ||
| }); | ||
|
|
||
| // TODO: print a better result |
There was a problem hiding this comment.
I removed this just to make it compile. We should reintroduce better messages.
There was a problem hiding this comment.
I think there is a rugged simplicity to stringified result.
There was a problem hiding this comment.
I have added a basic console.log print out of results leveraging the same helper message as is used in the ignition helper exception code.
| @@ -14,6 +10,15 @@ import { IgnitionHelper } from "../src/ignition-helper"; | |||
| import { clearPendingTransactionsFromMemoryPool } from "./execution/helpers"; | |||
| import { waitForPendingTxs } from "./helpers"; | |||
|
|
|||
| declare module "mocha" { | |||
There was a problem hiding this comment.
@kanej I tried to type these helpers to be able to debug some things, but I don't think the other deploy helper can be typed.
To be honest, I find creating this kind of helpers really hard to reason, especially since they have type any unless you type them.
There was a problem hiding this comment.
I have no problem pulling them out as just standard helper functions. Is that your preference?
There was a problem hiding this comment.
I have pushed a temporary typing. I split out the two deploys (file based and ephemeral), one is now runControlledDeploy and is mainly used in the execution that is currently skipped.
I have no problem pulling these helpers out/replacing the tests with a different structure. We talked about moving the execution tests currently skipped into core, which may simplify the integration tests enough this issue goes away.
|
I accidentally broke a plugin test. That's confusing me a ton. I'll debug it early tomorrow. |
|
Also, I think this works nicely with TypeChain, but I still need to test it. |
The stage two validation now returns either a validation error result or null. Internally the validtion continues to throw validation errors, so only one error will be reported at a time. However the types now match the return result so we can improve this in the future.
| // however, by taking only the top level submodules of the current depth and recursively | ||
| // validating each of those, we achieve the same effect. | ||
| const submodules = module.submodules; | ||
| for (const submodule of submodules) { |
There was a problem hiding this comment.
Get futures from module, gets the futures of all submodules as well recursively. Have I missed a case that submodule recursion gets us here?
There was a problem hiding this comment.
Sounds right! We should have a small test for this though
There was a problem hiding this comment.
Whoever does the validation multi-result work can add tests around this as well.
| Submodule: { | ||
| depositValue: BigInt(this.hre.ethers.parseEther("1")), | ||
| parameters: { | ||
| Submodule: { |
Adds a basic exception message from the Ignition helper for error deployment results. Adds a few basic tests to let us iterate on the messages.
Leverage the result to error message function and a new display success message function to show the deployment result at the command line.
alcuadrado
changed the title
No description provided.