stellar / stellar-cli

CLI for Soroban contracts.
61 stars 50 forks source link

Add a `--js-stellar-sdk-version` option to `soroban contract bindings typescript` #1327

Open leighmcculloch opened 2 months ago

leighmcculloch commented 2 months ago

I think we should add a --js-stellar-sdk-version option to soroban contract bindings typescript.

When folks are generating contract bindings for their contract they're generating bindings that'll be used with a version of the js-stellar-sdk that they may already have imported into their application.

Today the version of CLI installed is tightly coupled to a version of the js-stellar-sdk, and upgrading one result in getting a version of the other.

It seems unreasonable to me that if a dev wants a latest CLI to get some new CLI feature, that they also need to go update the applications they use if they need to regenerate their bindings.

I think it's reasonable to limit the range of support. The CLI shouldn't be expected to support all versions of js-stellar-sdk going back to the beginning, but as part of addressing this issue a general rule can be decided on, such as support the last two major versions of the js-stellar-sdk.

cc @chadoh cc @Shaptic @mollykarcher

leighmcculloch commented 2 months ago

I can also see an argument for specifying a version of the bindings instead, where a version of the bindings are still one-to-one mapped with the js-stellar-sdk. That solution would also address the same concerns, and would allow us to talk more specifically about changes to the bindings over time.

Shaptic commented 2 months ago

that'll be used with a version of the js-stellar-sdk that they may already have imported into their application

This is a fundamentally incorrect thing to do. The only way someone should access stellar-sdk is transitively via the bindings if they are using them because they are not cross-compatible.

if they need to regenerate their bindings

This is an opt-in action, right? So I don't see the concern. The only time it's compulsory is at protocol upgrades.

leighmcculloch commented 2 months ago

that'll be used with a version of the js-stellar-sdk that they may already have imported into their application

This is a fundamentally incorrect thing to do. The only way someone should access stellar-sdk is transitively via the bindings if they are using them because they are not cross-compatible.

Why is it unreasonable for someone to import the js-stellar-sdk for some uses, as well as use a binding to call a contract?

if they need to regenerate their bindings

This is an opt-in action, right? So I don't see the concern. The only time it's compulsory is at protocol upgrades.

We've seen folks update CLI, then as part of development regenerating their client (maybe they add a new function), then be forced to update the js-stellar-sdk when they just wanted to update their bindings.

The connection to CLI version is an oddity, and the experience gets worse the more dapps you work on because they may not all be using the same version.

Shaptic commented 2 months ago

Why is it unreasonable for someone to import the js-stellar-sdk for some uses

It's not, but they should be importing it from the bindings themselves!

We've seen folks update CLI, then as part of development regenerating their client (maybe they add a new function), then be forced to update the js-stellar-sdk when they just wanted to update their bindings.

But if they regenerate their bindings, it will automatically pull the appropriate version of the SDK. As long as they aren't separately installing the SDK (again, they shouldn't be because they aren't compatible with the bindings' imported SDK), it should still be transparent as long as it's not a protocol boundary (i.e. a breaking set of API changes).

leighmcculloch commented 2 months ago

But if they regenerate their bindings, it will automatically pull the appropriate version of the SDK.

Just because they have a newer CLI doesn't mean they will want to upgrade the bindings or the SDK in use though.

chadoh commented 2 months ago

Right now, there is tight coupling between the specific TS Bindings generated by the CLI (that is, the small amount of JS code that must live in the CLI repo) and stellar-sdk. You really couldn't mix and match. We need to tightly lock the two together, with a specific stellar-sdk version in the package.json of the generated package like 12.0.0-rc.3 and NOT ^12.0.0-rc.3, otherwise things will not work as expected.

As the APIs stabilize, I would expect there to be some more flexibility. But right now, this "CLI depends on ContractClient in stellar-sdk" setup is incredibly new and unstable.

Just because they have a newer CLI doesn't mean they will want to upgrade the bindings or the SDK in use though.

If you change your contract interface, you will want to generate a new NPM package, so that your types match the new interface. If you've upgraded your CLI, then the generated NPM package will have new features and bug fixes, which for now probably rely on new features and bug fixes in stellar-sdk. Maybe it's ok, for now, to require them to upgrade stellar-sdk when they update the CLI?

Shaptic commented 2 months ago

Just because they have a newer CLI doesn't mean they will want to upgrade the bindings or the SDK in use though.

If you update your software you update every part of it :shrug: if you don't want the new bindings then go generate them with the old CLI.