-
Notifications
You must be signed in to change notification settings - Fork 1
Add deployed field with capture variable, multiple networks #1
base: main
Are you sure you want to change the base?
Conversation
as: "SafeMathLibTransactionHash" | ||
# stop execution and error if we don't get the variable | ||
# setting to false would mean we continue without it | ||
strict: true |
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.
🎨 Not super opinionated here, but between strict
and the alternative, loose
mentioned in the README, I personally prefer strict
. I'm not sure how to articulate it, but something about the capture
context here makes loose
feel potentially confusing. Open to push back though!
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 think I agree! It feels clearer. I think @gnidan had advocated for loose. Open to discussing it of course!
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.
can we make the capture
syntax a separate PR? seems like it's more complicated than we should include here
# put npm packages needed for deployment here | ||
require: ["dotenv"] | ||
# organize by network, then by | ||
deployed: |
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 know there was conversation around potentially letting users define various states: (deployed
, maintenance
, beta
etc), but I'm wondering if we can just make the deployed
state our MVP and allow for custom states later.
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 think that is reasonable. Then the initial solver can just deal with the base state of deployed
. I'm not sure I'd want the other options at all honestly -- a user can version their yaml
files and have the deployed
state be something different in each version maybe. It feels like what is "deployed" is the main concern of the solver and these other states could muddy that. Definitely open to discussing that further but we can probably all agree to start with deployed!
HumanStandardToken: | ||
# setting ifTrue tells the solver not to proceed with this deployment unless | ||
# we have the designated capture variable. ifTrue could also be a list/array | ||
ifTrue: SafeMathLibAddress |
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.
❓ Since we will be supporting strict
from the SafeMathLib
contract side, do we need to support this conditional logic from dependents of SafeMathLib
? I guess having both would offer flexibility for a scenario like:
SomeLibAddress strict: false
to ignore deploy failures (perhaps to allow non-dependant contracts to continue deploying), but then SomeDependent
set to ifTrue: SomeLibAddress
blow up.
arguments: | ||
- SafeMathLib | ||
- options: | ||
gasPrice: 15000000 |
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.
gasPrice: 15000000 | |
gasPrice: 15000000 |
as: "SafeMathLibTransactionHash" | ||
# stop execution and error if we don't get the variable | ||
# setting to false would mean we continue without it | ||
strict: true |
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.
can we make the capture
syntax a separate PR? seems like it's more complicated than we should include here
version: 0.0.1 | ||
description: declarative deployment file "declarative deployments for Truffle projects | ||
# put npm packages needed for deployment here | ||
require: ["dotenv"] |
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.
not sure this belongs here
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.
how do you see the flow for requiring external packages? do you not think it should be in the declaration yaml file? this was a first pass at doing that
@@ -0,0 +1,38 @@ | |||
version: 0.0.1 |
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.
what does this version field refer to?
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 was thinking the version of the DApp? I guess we should talk about what versions of what need to be indicated here.
@@ -0,0 +1,38 @@ | |||
version: 0.0.1 | |||
description: declarative deployment file "declarative deployments for Truffle projects |
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.
what's the purpose of this description field?
This PR adds a
deployed
element to ouryaml
file, organized by network.A couple of other features included here: