-
Notifications
You must be signed in to change notification settings - Fork 0
Closes AP-752,AP-779: Use cli types in tasks #392
Conversation
@@ -5,7 +5,7 @@ import {getAddresses, getSigners, logger, structToObject} from "utils"; | |||
type TaskArgs = {id: number}; | |||
|
|||
task("manage:queryEndowments", "Will query an Endowment's details") | |||
.addParam("id", "the endowment id", 0, types.int) | |||
.addParam("id", "the endowment id", undefined, types.int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to query for non-existent endow IDs
|
||
type TaskArgs = { | ||
appsSignerPkey?: string; | ||
endowType: 0 | 1; | ||
endowType: EndowmentType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this task's logic, it seems with this small type change it finally fully supports creating DAFs too
(see Linear issue https://linear.app/angel-protocol/issue/AP-752/add-support-for-creating-dafs-in-managecreateendowment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 😎
AP-779 Create custom task argument types (where applicable)
An example would be Use /tasks/types.ts as inspiration AP-752 Add support for creating DAFs in `manage:createEndowment` |
.addParam( | ||
"gas", | ||
"Qty of tokens fwd'd to pay for gas. Make sure to use the correct number of decimals!", | ||
0, | ||
undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes us be explicit about the value we use
@@ -16,98 +17,37 @@ type TaskArgs = Partial<RegistrarMessages.UpdateConfigRequestStruct> & { | |||
yes: boolean; | |||
}; | |||
|
|||
// TODO: update param descriptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated param descriptions
} catch (error) { | ||
logger.out(error, logger.Level.Error); | ||
if (taskArgs.acceptedTokens.length === 0) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduce nesting
@@ -14,7 +14,6 @@ import { | |||
} from "utils"; | |||
|
|||
type TaskArgs = { | |||
factory?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a situation where we'd be deploying a new EndowmentMultiSigFactory
without storing its address in contract-address.json
and then using this new contract in for this task.
In other words, we'd be reading the appropriate address from the address file in (almost?) all cases (as is the assumption in other tasks)
Explanation of the solution
Instructions on making this work
yarn
oryarn install
to install npm dependencies