Deploy through Accounts

[ericnordelo]

Fixes #259, #277

[andrew-fleming]

Looking really good, @ericnordelo! I left a few comments.

Also, I think we should add some tests for deploy_contract from deploy.py and account.py. We can probably reuse most of the existing deploy tests with some adjustments.

[andrew-fleming]

Looking good! I left some comments :)

Some other notes:

• I think we need to remove line #85 in deployments: https://github.com/ericnordelo/nile/blob/feat/deploy-through-account-%23259/src/nile/deployments.py#L84-L85
• It might be a good idea to document the requirement of declaring contracts prior to deploying them. At this early stage, I'm not sure it's obvious to do so.

[andrew-fleming]

What do you think about including a random salt generator for deployments without --salt like StarkWare does? => https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/starknet/cli/starknet_cli.py#L846-L847

Otherwise, trying to deploy multiple instances of the same contract will result in:

raise StarkException(code=code, message=message) starkware.starkware_utils.error_handling.StarkException: (500, {'code': <StarknetErrorCode.CONTRACT_ADDRESS_UNAVAILABLE: 1>, 'message': 'Requested contract address 1999693730423262048654525999365022023777068478432229193013554800567845966099 is unavailable for deployment.'})

Users can add salt to avoid this, but automating salt seems preferable

[andrew-fleming]

If we agree^, I definitely suggest adding some salt tests

[ericnordelo]

I think this salt should be part of the protocol (like nonce in Ethereum deployments), but I am not sure why it is left to be passed from the end users (am I missing an important reason?). With this said, agreeing on a format for the automatic generation of this salt, I think, is content enough for another issue/PR. Shouldn't be any problem with releasing this version, right? I don't see a reason to delay it to another milestone.

[andrew-fleming]

I think this salt should be part of the protocol

I agree in general haha but since we're deploying contracts through an account.send tx, the burden seems to fall on Nile. When Nile interacts with the starknet_cli directly (and when Nile most likely bypasses the cli in the future), I imagine defining a salt generator will not be necessary

agreeing on a format for the automatic generation of this salt, I think, is content enough for another issue/PR

That sounds reasonable to me

I don't see a reason to delay it to another milestone.

I don't disagree

[martriay]

Thanks for the changes! This is almost done

[andrew-fleming]

The improvements look great! Making the API consistent with nile deploy <key> <contract> is much more intuitive, so kudos on the change!

I left a +1 to Marto's comment in removing the _tx on the signer methods. I don't think there's much to gain with it included.

Otherwise, this looks good to go for me :)

[ericnordelo]

I left a +1 to Marto's comment in removing the _tx on the signer methods. I don't think there's much to gain with it included.

The _tx can be removed, but the change I wanted to introduce was replacing sign_transaction by sign_invoke, because sign_transaction sounded like it could be used to sign declares, or deploy_account, transaction, and this is not the case (_tx was because it looked better to me atm sign_invoke_tx than sign_invoke)

[andrew-fleming]

_tx was because it looked better to me atm sign_invoke_tx than sign_invoke

That's fair...sign_invoke doesn't look that great. I don't mind sign_execute though as previously mentioned, but no objections by me if we keep it as is

[ericnordelo]

That's fair...sign_invoke doesn't look that great. I don't mind sign_execute though as previously mentioned, but no objections by me if we keep it as is

I chose invoke over execute to follow the starknet source code wording (as you can see here.)

[martriay]

I get the point, but Invoke is a legacy term for when you could call any arbitrary entry point. I don't think following legacy terminology adds much clarity to users now that invoke is just a proxy to call __execute__ with it.

That being said, I don't think it makes a big difference either, I lean towards execute but no strong opinions really.

[ericnordelo]

I get the point, but Invoke is a legacy term for when you could call any arbitrary entry point. I don't think following legacy terminology adds much clarity to users now that invoke is just a proxy to call execute with it.

That sounds fair, but we are still using invoke (not execute) in important parts of the project, like in logging for cli (with nile send for example, and other commands), in the README as well, or in the source code where we have call_or_invoke, and invoke type for transactions as some examples. I think sign_invoke is more consistent with what we have, but we could refactor this in a separate issue if you agree.

[martriay]

either sign_invoke or sing_execute are fine with me as long as we drop _tx. this is ready :)

[ericnordelo]

either sign_invoke or sing_execute are fine with me as long as we drop _tx. this is ready :)

Removed the _tx suffix :)

[andrew-fleming]

I think we're good to go!