-
Notifications
You must be signed in to change notification settings - Fork 201
Push only required contract during create command #1426
Conversation
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.
The change looks good, Igor! Could I ask you to add a few tests on test/scripts/push.test.js
? Thanks!
@@ -89,9 +94,9 @@ async function runActionIfRequested(externalOptions: any): Promise<void> { | |||
return action(options); | |||
} | |||
|
|||
async function runActionIfNeeded(contractName: string, network: string, options: any): Promise<void> { | |||
async function runActionIfNeeded(contractAlias: string, network: string, options: any): Promise<void> { |
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.
This contractName
was not even used, right?
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.
Correct.
if (contractAlias) pushArguments.contractAliases = [contractAlias]; | ||
|
||
if (!options.skipTelemetry) | ||
await Telemetry.report('push', (pushArguments as unknown) as Record<string, unknown>, interactive); |
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'm curious: why was this cast needed when changing from { [key: string]: unknown }
to Record<string, unknown>
in the report
method definition? I thought that both were semantically the same.
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.
The cast is needed not because of the change of report
method but because of change pushParams
to type PushParams
which doesn't have required index signature.
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.
LGTM!
@@ -122,13 +122,13 @@ export default class NetworkController { | |||
} | |||
|
|||
// DeployerController | |||
public async push(reupload = false, force = false): Promise<void | never> { | |||
public async push(aliases: string[], { reupload = false, force = false } = {}): Promise<void | never> { |
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.
aliases
should be string[] | undefined
, and the same in _contractsListForPush
. It's not necessary because we're not using strict mode yet, but I'd like us to be explicit about it so it will be easier to migrate to strict mode in the future.
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.
Yes. I was thinking there are all the null checks. I completely forgot about that our tsconfig
is not in a strict mode. We should bump it to it as fast as possible.
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.
These are proper tests. Great work Igor!!
I have updated cli sdk & upgrades to version 2.7.1. I have 2 different contracts let say Contract A {} and Contract B {}. Both the contracts are added and part of the contracts object of the project.json file. When I run oz create in interactive mode or oz create ContractA, it always deploys ContractA and ContractB together even after specifying only 1 contract which I need to deploy. Looks like it still not fixed with the new release or am I doing something wrong with create. |
Hi @darshan0071990. This was not included in the 2.7 release. It will be part of the 2.8 release. |
Fixes #1250. Will push only request contract by
create
command. The same functionality theoretically can be implemented forupdate
command as well but it will require quite a bit of rearchitecting due toupdate
command accepting options likeall/alias/address
as well as these options being resolved down the call stack of theupdate
command.