Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Nov 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bbe4e1
XDR side of the contract deployment changes specified in CAP-46-02.
dmkozh Oct 24, 2022
9177fbc
Remove option to create and install contract in the same operation.
dmkozh Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Stellar-contract.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,14 +250,14 @@ case NEGATIVE:

enum SCContractCodeType
{
SCCONTRACT_CODE_WASM = 0,
SCCONTRACT_CODE_WASM_REF = 0,
SCCONTRACT_CODE_TOKEN = 1
};

union SCContractCode switch (SCContractCodeType type)
{
case SCCONTRACT_CODE_WASM:
opaque wasm<SCVAL_LIMIT>;
case SCCONTRACT_CODE_WASM_REF:
Hash wasm_id;
case SCCONTRACT_CODE_TOKEN:
void;
};
Expand Down
20 changes: 18 additions & 2 deletions Stellar-ledger-entries.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ enum LedgerEntryType
CLAIMABLE_BALANCE = 4,
LIQUIDITY_POOL = 5,
CONTRACT_DATA = 6,
CONFIG_SETTING = 7
CONTRACT_CODE = 7,
CONFIG_SETTING = 8
};

struct Signer
Expand Down Expand Up @@ -499,6 +500,13 @@ struct ContractDataEntry {
SCVal val;
};

struct ContractCodeEntry {
ExtensionPoint ext;

Hash hash;
opaque code<SCVAL_LIMIT>;
};

enum ConfigSettingType
{
CONFIG_SETTING_TYPE_UINT32 = 0
Expand Down Expand Up @@ -560,6 +568,8 @@ struct LedgerEntry
LiquidityPoolEntry liquidityPool;
case CONTRACT_DATA:
ContractDataEntry contractData;
case CONTRACT_CODE:
ContractCodeEntry contractCode;
case CONFIG_SETTING:
ConfigSettingEntry configSetting;
}
Expand Down Expand Up @@ -622,6 +632,11 @@ case CONTRACT_DATA:
Hash contractID;
SCVal key;
} contractData;
case CONTRACT_CODE:
struct
{
Hash hash;
} contractCode;
case CONFIG_SETTING:
struct
{
Expand All @@ -645,6 +660,7 @@ enum EnvelopeType
ENVELOPE_TYPE_CONTRACT_ID_FROM_ED25519 = 8,
ENVELOPE_TYPE_CONTRACT_ID_FROM_CONTRACT = 9,
ENVELOPE_TYPE_CONTRACT_ID_FROM_ASSET = 10,
ENVELOPE_TYPE_CONTRACT_ID_FROM_SOURCE_ACCOUNT = 11
ENVELOPE_TYPE_CONTRACT_ID_FROM_SOURCE_ACCOUNT = 11,
ENVELOPE_TYPE_CREATE_CONTRACT_ARGS = 12
};
}
79 changes: 68 additions & 11 deletions Stellar-transaction.x
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,23 +474,66 @@ struct LiquidityPoolWithdrawOp
int64 minAmountB; // minimum amount of second asset to withdraw
};

enum HostFunction
enum HostFunctionType
{
HOST_FN_INVOKE_CONTRACT = 0,
HOST_FN_CREATE_CONTRACT_WITH_ED25519 = 1,
HOST_FN_CREATE_CONTRACT_WITH_SOURCE_ACCOUNT = 2,
HOST_FN_CREATE_TOKEN_CONTRACT_WITH_SOURCE_ACCOUNT = 3,
HOST_FN_CREATE_TOKEN_CONTRACT_WITH_ASSET = 4
HOST_FUNCTION_TYPE_INVOKE_CONTRACT = 0,
HOST_FUNCTION_TYPE_CREATE_CONTRACT = 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

HOST_FUNCTION_TYPE_INSTALL_CONTRACT_CODE = 2
};

enum ContractIDType
{
CONTRACT_ID_FROM_SOURCE_ACCOUNT = 0,
CONTRACT_ID_FROM_ED25519_PUBLIC_KEY = 1,
CONTRACT_ID_FROM_ASSET = 2
};

enum ContractIDPublicKeyType
{
CONTRACT_ID_PUBLIC_KEY_SOURCE_ACCOUNT = 0,
CONTRACT_ID_PUBLIC_KEY_ED25519 = 1
};

struct InstallContractCodeArgs
{
opaque code<SCVAL_LIMIT>;
};

union ContractID switch (ContractIDType type)
{
case CONTRACT_ID_FROM_SOURCE_ACCOUNT:
uint256 salt;
case CONTRACT_ID_FROM_ED25519_PUBLIC_KEY:
struct
{
uint256 key;
Signature signature;
uint256 salt;
} fromEd25519PublicKey;
case CONTRACT_ID_FROM_ASSET:
Asset asset;
};

struct CreateContractArgs
{
ContractID contractID;
SCContractCode source;
};

union HostFunction switch (HostFunctionType type)
{
case HOST_FUNCTION_TYPE_INVOKE_CONTRACT:
SCVec invokeArgs;
case HOST_FUNCTION_TYPE_CREATE_CONTRACT:
CreateContractArgs createContractArgs;
case HOST_FUNCTION_TYPE_INSTALL_CONTRACT_CODE:
InstallContractCodeArgs installContractCodeArgs;
Comment on lines +527 to +530
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

struct InvokeHostFunctionOp
{
// The host function to invoke
HostFunction function;

// Parameters to the host function
SCVec parameters;

// The footprint for this invocation
LedgerFootprint footprint;
};
Expand Down Expand Up @@ -580,23 +623,37 @@ case ENVELOPE_TYPE_POOL_REVOKE_OP_ID:
case ENVELOPE_TYPE_CONTRACT_ID_FROM_ED25519:
struct
{
Hash networkID;
uint256 ed25519;
uint256 salt;
} ed25519ContractID;
case ENVELOPE_TYPE_CONTRACT_ID_FROM_CONTRACT:
struct
{
Hash networkID;
Hash contractID;
uint256 salt;
} contractID;
case ENVELOPE_TYPE_CONTRACT_ID_FROM_ASSET:
Asset fromAsset;
struct
{
Hash networkID;
Asset asset;
} fromAsset;
case ENVELOPE_TYPE_CONTRACT_ID_FROM_SOURCE_ACCOUNT:
struct
{
Hash networkID;
AccountID sourceAccount;
uint256 salt;
} sourceAccountContractID;
case ENVELOPE_TYPE_CREATE_CONTRACT_ARGS:
struct
{
Hash networkID;
SCContractCode source;
uint256 salt;
} createContractArgs;
};

enum MemoType
Expand Down