Add support for typed constructor calls in tests (`register`) and deployed code (`deploy`)

[leighmcculloch]

What

Add support for typed constructor calls in tests (register) and deployed code (deploy)

Why

See:

• https://github.com/stellar/rs-soroban-sdk/pull/1343#issuecomment-2364741525

[leighmcculloch]

I think a way we could do this is to add an associative type to the ContractRegister trait.

The SDK generates an implementation for the ContractRegister trait for each #[contract] struct.

The associated type would be required to be IntoVal<Vec<Val>>.

For a contract that accepts no args, the associated type would simply be ().

For a contract that accepts args, the associated type would be specified as a tuple of the types the contract requires.

The register functions would then reference the associated type as the required type for the args, and then internally convert them to the Vec.

We could do the same thing with the deploy function.

However, in both these cases we need someway to provide an -out-, if say we're loading a contract and it isn't known what types it accepts, because maybe it doesn't have a spec. We still have to support that case, but I'm not sure if we can support both the typed and untyped approaches.

I'll have a play with this idea and see what's possible.

[dmkozh]

The 'untyped' approach must remain at least for the deploy scenario in order to allow for dynamic dispatch.

I'm not sure if that's what you're already proposing, but could we generate fn __constructor(env: Env, ctor_arg1: T1, ...) -> Self function on every contract client in test builds? This might seem a bit narrow (because we don't allow registering at a fixed address), but I think it's a nicer alternative to the current syntax in majority of tests.

[leighmcculloch]

I started down the approach of using associated types (1‍⃣) in https://github.com/stellar/rs-soroban-sdk/pull/1369, and it does look like the experience of using it would be sweeeeet. However I've run into a problem that defining the types on the associated types is done in #[contract] but we don't now what the types are until #[contractimpl] is called. I don't think I can make this work without significantly changing the way the macros work, or making them intertwined, or doing something not great like saying a constructor must always be provided.

Considering other ideas:

---

2‍⃣ –

could we generate fn __constructor(env: Env, ctor_arg1: T1, ...) -> Self function on every contract client in test builds

I think we should do it for non-test builds too, so the deploy and register experiences are the same. In your example, the function returns Self, so is the idea that it is what we use to construct the client? Could you share an example of how the function gets used to register and deploy?

---

3‍⃣ – Another idea I was thinking about was for every contract function, we add a second function, an _args variant, or __args if we want to reduce the chance of collissions with existing functions. The _args variant would take in the typed arguments, and return a impl IntoVal<Env, Vec<Val>>.

The register call would look like this:

let contract_id = env.register(
    Contract,
    Contract::_constructor_args(v1, v2, v3),
);

And the deploy call would look like this:

env.deployer()
    .with_current_contract(salt)
    .deploy_v2(
        wasm_hash,
        Contract::_constructor_args(v1, v2, v3),
    );

Conveniently the signature of the register and deploy functions don't change and can be easily used for dynamic dispatch.

Inconveniently you have to discover the _args functions yourself or via examples. The compiler won't help you find it, and it requires the additional syntax rather than just accepting the values.

[dmkozh]

I think we should do it for non-test builds too, so the deploy and register experiences are the same.

Deploy and register are quite different though. Register is test-only, needs to support compiled-in contracts and doesn't have to support dynamic dispatch. Deploy needs to support dynamic dispatch, is wasm-only and is more sensitive to the amount of code generated. It would be great if we could converge these, but I don't think it's a hard requirement.

In your example, the function returns Self, so is the idea that it is what we use to construct the client?

Yeah, I think for tests it's actually more intuitive. Maybe the name should be different though, like __register. So it would look like let token = TokenClient::__register(env, admin, symbol, decimals);. In theory, we could also have let token = TokenClient::__register_wasm(env, wasm, admin, symbol, decimals);, but we'd still have to keep the dynamic dispatch methods around.

On a side note, I've just realized that our clients have new method already that might collide with the contract functions, not sure if we could continue this pattern by doing something like deploy_new/new_deployed.

[leighmcculloch]

I've just realized that our clients have new method already that might collide with the contract functions

Since it's existed for years, I don't think we need to do anything immediately. If it shows up as a pain point, we can introduce a call_new. It's an unfortunate thing with the overlapping APIs. At the least the new function is without self, and all the call functions are with self, so they aren't easily confused. Anyone writing a contract in Rust should see an error if they name a function new.

[leighmcculloch]

Register is test-only, needs to support compiled-in contracts and doesn't have to support dynamic dispatch.

Register does need to support dynamic dispatch for .wasm files loaded.

[leighmcculloch]

TokenClient::__register

This approach ☝ would suffer from the same challenge that https://github.com/stellar/rs-soroban-sdk/pull/1369 did: generating the __register function is challenging when the types it'll used need to come from a constructor function, or from the lack of a constructor function. It's that "something can be defined in two places, where the second place is non-existence" that's tricky to do in Rust macros.

[leighmcculloch]

I've opened a change that I'm experimenting on idea 3‍⃣ with. It ended up looking a little different:

let contract_id = env.register(
    Contract,
    ContractArgs::__constructor(v1, v2, v3),
);

env.deployer()
    .with_current_contract(salt)
    .deploy_v2(
        wasm_hash,
        ContractArgs::__constructor(v1, v2, v3),
    );

• https://github.com/stellar/rs-soroban-sdk/pull/1382

The biggest downside of 3‍⃣ is discoverability 😕.