-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
Add register
and register_at
fns to replace register_contract
, register_contract_with_constructor
, register_contract_wasm
, register_contract_wasm_with_constructor
#1343
base: main
Are you sure you want to change the base?
Conversation
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I like this a lot.
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.
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.
I suppose we could have a special struct that has |
I'll open a separate PR with the breaking change to the deploy fns. |
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. |
What
Add
register
andregister_at
fns.Deprecate
register_contract
andregister_contract_wasm
.Unexport
register_contract_with_constructor
andregister_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_contract
calls 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 #1341
Known limitations
It would be better imo if we could add the constructor arguments to the existing
register_contract
andregister_contract_wasm
fns, but that would be a breaking change.The
deploy_with_constructor
functions still exist which is inconsistent, but harder to align because to change thedeploy
function 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?