XDR side of the contract deployment changes specified in CAP-46-02.

[dmkozh]

This introduces the ledger entries to store the wasm code and also changes the host functions to support creating/installing the contracts.

See stellar/stellar-protocol#1315 for the general overview of the changes.

[leighmcculloch]

The structure of the new types is somewhat ambiguous as to how to use them. Some questions inline about it.

[leighmcculloch]

This relationship feels a bit backwards. It suggests a source account is a public key, when a source account contains a public key. Could "source account" be a ContractIDType variant instead? Then you don't need as much nesting. Nesting comes at a XDR size cost, and hierarchies make more assumptions about the future. It doesn't appear to offer a ton of value in this case. Am I missing something of value there?

Suggested change

	enum ContractIDType
	{
	CONTRACT_ID_FROM_PUBLIC_KEY = 0,
	CONTRACT_ID_FROM_ASSET = 1
	};
	enum ContractIDPublicKeyType
	{
	CONTRACT_ID_PUBLIC_KEY_SOURCE_ACCOUNT = 0,
	CONTRACT_ID_PUBLIC_KEY_ED25519 = 1
	};
	enum ContractIDType
	{
	CONTRACT_ID_FROM_SOURCE_ACCOUNT = 0,
	CONTRACT_ID_FROM_PUBLIC_KEY_ED25519 = 1,
	CONTRACT_ID_FROM_ASSET = 2
	};

[dmkozh]

I use nesting for the sake of avoiding duplication and to represent the logical structure of the data. Any kind of id inherited from a public key would contain salt, but the source of the public key itself might be different. If we add one more type of a public key in the future, it would just go into ContractIDPublicKeyType enum and will by definition have the salt.

I'm not feeling too strongly about that though and I agree the difference is pretty marginal. If that apparently doesn't improve clarity, we can remove the nesting as well.

[leighmcculloch]

Any kind of id inherited from a public key would contain salt

This is an assumption. I don't think we know if it would be true for all ways we create contracts from public keys. This is what I mean by:

hierarchies make more assumptions about the future

[leighmcculloch]

The two ways to do a thing is confusing to me, and I struggle to see us actually promoting both ways in docs. See inline comment.

[leighmcculloch]

It feels like an unnecessary optimization to support two ways to install contracts. Developers are going to want to be told one way to do it for 99% of the time. In our docs, we will want to describe one way and not have to tell a long story about when to use either way.

If installing separately is a critical feature we need, could we make that the only way to upload code? Two ops are a small cost, so we should instead make it trivial to do two ops, one install, one create.

If the ref is just a sha256, that's a small lift to require developers to provide in the second operation.

[dmkozh]

I'm still a bit concerned about the multi-op transactions and creating a contract with a single operation seems important enough to have this as a precaution if we come back to single op.

Adding or removing this seems pretty easy, I would probably keep this around until we have decided on multi-op txs, but I could remove this for now if you feel strongly about it.

[leighmcculloch]

I defer to @sisuresh.

But if we are concerned about multi-op transactions, then it feels like the contract lifecycle story as a whole has some holes, because last I remember we're planning to rely on multi-op for atomic initialization. Or is that still undecided?

[sisuresh]

I believe the potential risk with multi-op transactions is that the fee model won't work or will become more difficult to get right. For this reason, we should be careful about making decisions off of this. We may need to add functionality for atomic initializations if the multi-op experiment doesn't work out.

But if we do want a single way to install a contract, why do both operations need to be in the same transaction? The install and create need to run sequentially, but not atomically, so you could use two separate transactions instead right?

[sisuresh]

This may actual expose my lack of knowledge with Horizon - Is it easy to "batch" submit two transactions? Or is the typical flow submit tx1 -> ack tx1 in ledger -> submit tx2?

[dmkozh]

But if we do want a single way to install a contract, why do both operations need to be in the same transaction?

I guess that's mostly a UX thing here.

Horizon should I think deliver both transactions into the same ledger if it can,

It can do this now because we perform a significant effort to allow multiple txs from the same account per ledger. That's pretty hard to maintain and given that eventually we may want to execute soroban txs in parallel, we may not even start supporting that behavior for soroban txs even in V1. That's still TBD, but definitely something to keep in mind.

Given the feedback, I'll remove this for now. Returning both atomic installation and initialization will be a simple incremental change if we need it.

[leighmcculloch]

Hmm, this is helpful perspectives. I think separate operations for install and create only work if we can do them in a single tx from a DX point-of-view. Maybe the unnecessary piece is the ability to install without creating, even though that sounds more logically pure.

Given the feedback, I'll remove this for now.

So it's clear, which piece is being removed?

[dmkozh]

I've removed the fused install option for now. I agree we should come back to it if we can't make multi-op work (the same applies to the initializers).

While install without create doesn't seem to be needed in the most of the cases, it supports the contract factories in a pretty clean fashion: you would need to install the source(s) to instantiate with the factory, install the source of the factory, then instantiate the factory contract (preferably all in the same tx if we can make it). So if we don't have a separate, then the developer would need to instantiate some throw-away contracts. I don't have intuition on usefulness of the factories (given that a non-empty set of their use-cases is already covered by decoupling the source from instance), but since we can easily come up with a use case, it doesn't feel right to make it awkward.

[dmkozh]

FWIW I wouldn't feel too bad if we had two options and promoted the fused option for the general use case, but have a separate documentation on 'contract factories' that refers to install specifically.

[leighmcculloch]

Ok, yeah this sounds fine and we can iterate on this again if the multiop doesn't work out.

[sisuresh]

If I understand this correctly, we can call these three "host functions" from InvokeHostFunctionOp, but contracts won't necessarily have the same access. For example, it doesn't sound like contracts have access to HOST_FUNCTION_TYPE_CREATE_CONTRACT. Does the naming here make sense then? Should HOST_FUNCTION still be used here?

[dmkozh]

That's something I've been wondering about too. Currently we have this referring actual host functions that are exposed through the env, but are prohibited to be called from the contracts. I think that is confusing. Now I'm using XDR which is easier to pass around (given any sort of complexity in argument structure), but also now I wouldn't even expose anything via env (which I think makes sense given that it wouldn't be usable anyway). So indeed calling this 'host function' feels weird.

I see the following options:

• With the current auth approach I would probably rename this to something else to not create confusion. Not sure what though. Also both this and 'invoke' would need to be renamed
• If we unify the auth approach, then we will be able to just expose the same functions to the contracts, so they would be able to perform the same operations. Then the naming will be legitimate. But that's contingent on where we end up with auth.

FWIW I'm not a big fan of the op name in either case (as it seems overly specific to implementation vs referring to smart contracts). Any ideas on better names?

[leighmcculloch]

The invoke-host-function-invoke-contract-function nested concept is not particularly fun to explain to folks either in person, or in docs.

I don't feel strongly about where this goes, but I suspect from a developer experience point-of-view if we are adding XDR types – which is attractive – we could considering having separate operations for each Soroban operation, which falls in line with historically what we've done, and has some benefits for ingesting applications that categorize based on operation type (this isn't something that needs to drive any decision, just noting alignment).

[sisuresh]

We could just remove the HOST_FUNCTION_TYPE*. You've already done a good job of abstracting away the host function details by no longer requiring the user to pass the exact ScVec parameters that get passed to the host function. So why even keep the concept of host functions here?

Separate operations is an interesting idea, and one that would be easy to do. One of the advantages of a single op with different options is that we don't need to plumb each new operation through core as we iterate. This is not that big of a deal, so I'm not opposed to separate operations if we see the benefit.

[dmkozh]

Separate ops is something to consider before V1, but that's a relatively significant change from the core standpoint (in terms of diffs etc, not necessarily in terms of complexity). I would propose to not try doing that within this PR to keep the reviews focused on one thing.

[sisuresh]

I agree - separate ops should not be part of this PR and require further discussion.

[dmkozh]

I would actually suggest to also do the renaming separately for the same reason (unless we only rename the enum; but then the op name itself will be confusing). There is a significant amount of diffs in core/host related only to the deployment changes and I don't want to additionally mix it with no-op renaming.

[sisuresh]

Makes sense to me.

[dmkozh]

Since we seem to have agreed to keep the naming for now and do the renaming separately, is there anything else that needs to be addressed before merge?

[sisuresh]

@dmkozh LGTM