Closed leighmcculloch closed 2 months ago
leighmcculloch
commented
2 months ago @dmkozh @sisuresh Thoughts on this change?
I'd love to make a change like this to reorient these functions around their actual use, as well as bring the deploy fns into consistency, however the only way to do the deploy functions would be with a breaking change.
Do you see a way to do deploy without a breaking change?
dmkozh
commented
2 months ago It would be better imo if we could add the constructor arguments to the existing register_contract and register_contract_wasm fns, but that would be a breaking change.
I honestly prefer the way we do this here, i.e. a single function that works for all the contract types. I think this has a long-term benefit of having a smaller and simpler interface. Basically it's not even necessary for a developer to understand the difference between wasm/non-wasm contracts, which I think is good (as the difference is mostly relevant only for the budget estimations). Moving the constructor to only the new interface seems like a good compromise between not breaking the existing users and encouraging usage of the new functions.
The deploy_with_constructor functions still exist which is inconsistent, but harder to align because to change the deploy function to always accept constructor args would be a breaking change.
Maybe we should actually go with a breaking change? Unlike the existing tests, where the breakage would be just a nuisance, it might be beneficial to explicitly break the contracts that do the deployment, as there is a good chance they will stop working correctly (if the deployed contract has non-trivial constructor). The scope of breakage will also be much smaller. We can publish the migration guide alongside the release notes. The change will also have a long-term benefit of reduced probability of messing up the factory contracts due to not supporting constructor.
This PR doesn't touch the register stellar asset contract functions at all. Ideas?
I suppose we could have a special struct that has admin field and have a Register implementation for it that registers SAC. But I'm not sure how to pass the necessary data alongside with the Address that we normally return from register. I guess we could maintain a map of registered addresses to SAC data, so that one could do something like let sac_address = env.register(StellarAssetContractImpl::new(admin), ()); let sac_client = StellarAssetContractClient::new(&env, &sac_address);. I'm not sure though if a bit more consistent solution warrants more complex and brittle implementation. I'm also not a fan of supporting register_at for SAC, as that would require messing with the SAC contract id generation logic (and will break the programmatic access to SAC address via get_asset_contract_id host fn). So I'd say even if we do the thing above, we would still want to fail when StellarAssetContractImpl is passed to register_at.
leighmcculloch
commented
2 months ago I'll open a separate PR with the breaking change to the deploy fns.
leighmcculloch
commented
2 months ago I think it's worth us trying to make typed invocation of constructors. It's so easy to pass the wrong type, especially with integers.
dmkozh
commented
2 months ago I think it's worth us trying to make typed invocation of constructors.
Maybe, but this would require us to build the registration/deployment functionality into the generated contract clients (I don't really see a different way to achieve this). I think both options should be available though, as the client is not always there and we generally want to support dynamic dispatch (even in the test environment). Something like MyContractClient::register(<ctor_args>) would be neat, but there is a risk of clashing with a contract function with the same name. Maybe it could be named __register, but that's quite ugly and not friendly to the Rust style. Do you have a better idea?
leighmcculloch
commented
2 months ago MyContractClient::register(
)
If we named the function constructor_args it's less likely to clash.
I'd be inclined to create a new type however, such as MyContractDeployer.
leighmcculloch
commented
2 months ago I'm merging this change, and we can explore the typed interface, and deploy fn separately. Opening issues for those:
What
Add
registerandregister_atfns.Deprecate
register_contractandregister_contract_wasm.Unexport
register_contract_with_constructorandregister_contract_wasm_with_constructor.Why
To reorient the register functions around what we expect the common cases to be:
It's always been odd that the first parameter to most
register_contractcalls isNone. It's confusing to look at the first time you see it, and for the most part most developers do not need to set a value for the contract id.Most contracts need initialization, and therefore I think most contracts over time will gain constructors.
To do so in a way that is backwards compatible with previous releases.
Close https://github.com/stellar/rs-soroban-sdk/issues/1341
Known limitations
It would be better imo if we could add the constructor arguments to the existing
register_contractandregister_contract_wasmfns, but that would be a breaking change.The
deploy_with_constructorfunctions still exist which is inconsistent, but harder to align because to change thedeployfunction to always accept constructor args would be a breaking change.It's possible to update code that uses the existing register functions using a regex find and replace, but it's not the most trivial regex to work out. If folks wish to resolve the deprecation warning they'll need to update potentially every test they've written. That in itself is not a great developer experience.
The old and new way don't look that different, and so for existing Soroban developers, the change from the old to new way may be confusing at a glance.
This PR doesn't touch the register stellar asset contract functions at all. Ideas?