Discuss merging stellar-ruby-sdk and stellar-ruby-base

[JakeUrban]

This issue is for discussing the pros and cons of keeping the separation of stellar-ruby-sdk and stellar-ruby-base (a.k.a. sdk, base). Currently, everything not related to horizon is kept in base, while the sdk has the horizon hyperclient, transaction paging, and SEP10 helper functions.

Why would we want to keep these separate?

1. Change velocity
   • changes to the horizon/client code would likely outpace changes to the base code, which changes only when stellar-core changes. To me, This doesn't seem significant enough to merit keeping it separate.

Why would we want to merge them?

1. Reduced time-to-release
   • Having two projects means the base project has to release changes before the client can update for the same thing
2. Easier to reference code
   • Since both projects use the same Stellar module, its not always clear where the functionality is defined. For example, Stellar::Account is not in base, its in the sdk. This isn't a huge deal but its a bit annoying to have 2 projects open and figure out where a particular class is implemented.

I'll add more as I think of them.

@overcat I know the Python SDK was originally 2 separate projects, but everything was merged into py-stellar-base. Can you elaborate on what led to that decision?

[leighmcculloch]

Reduced time-to-release

I think this point is moot because ruby-stellar-sdk defines in its gemspec a dependency on any version of ruby-stellar-base greater than 0.18.0, so whenever new versions of ruby-stellar-base are released importers can update to them independent of upgrading ruby-stellar-sdk. If we fail to release updates to ruby-stellar-sdk, importers still get updates to `ruby-stellar-base.

while the sdk has the ... SEP10 helper functions

We probably should have put the SEP-10 helpers in ruby-stellar-base because it isn't dependent on Horizon. For me this occurs as evidence that having them separate is too much effort.

All in all I'm 👍 with merging them.

[JakeUrban]

FYI @abuiles @ire-and-curses @tomerweller @jimdanz

[jimdanz]

Thanks for the mention. +1 to merging them. As a user, the main inconvenience has been needing to grep around in 2 different repos to find things.

[overcat]

The Python Stellar SDK has always had only one GitHub repository, but it has two PyPi package names. In the early stage, it was called stellar-base, and later I renamed it stellar-sdk, stellar-sdk is the successor to stellar-base.

Sorry, I just accidentally replied to this issue with my work account, and then I deleted it. 😅

[ire-and-curses]

I'm in favour also. There's a potential benefit if someone was going to use a pure tx construction library without the client, but that doesn't seem likely here, as the concerns are muddled. :+1: to combining them for simplicity.

[JakeUrban]

Should we move the base code into the SDK project? This way seems like it would be the least likely to cause issues, compared to moving the SDK into the base project.

[tomerweller]

I'm in favor. The separation was always a bit unclear. :+1: for base into sdk and not the other way around.

Let's be very careful about this process so that if someone relies on base directly they get ample warning.

[leighmcculloch]

According to rubygems there are significantly more downloads of stellar-base than there are of stellar-sdk in total, but for the most recent versions the sdk trumps the base, so it probably suggests that most people are using the sdk with the base rather than the base without the sdk.

https://rubygems.org/gems/stellar-sdk https://rubygems.org/gems/stellar-base

So I think moving things into the sdk is good.

Simply moving the code from one to the other will cause errors for folks who are already importing them though because they could end up with duplicate or redefined classes in two gems. I think we should consider a couple approaches:

1. Remove all the types and functions from base and release a new version. Add all the types and functions to sdk and also make the sdk gem require that new version of the base, so that anyone updating the sdk will update to a base that removes the conflicts. At some far future date remove the sdk depending on base at all.
2. Move all the types from base to sdk, but leave references to them in base so base will continue to function on its own. Also do the dependency update in (1) so that somebody doesn't use an old version of base with the new sdk. This would support people using base for longer.

[JakeUrban]

I just ran a little experiment with gems and their dependencies.

I created a tmpgem-base gem

• It had one Tmp class

I created a tmpgem-sdk gem

• It had one TmpSDK class
• required tmpgem-base

This is the current setup we have between the two projects.

Then, I put the Tmp class from tmpgem-base into tmpgem-sdk removed the tmpgem-base dependency from tmpgem-sdk Bumped the version for tmpgem-sdk No changes to tmpgem-base

Note that tmpgem-base is still installed in the environment

Then I ran:

require 'tmpgem-sdk'
TmpGem::Tmp

And it loaded successfully.

This means that removing the dependency from the sdk project and moving the code from base into sdk works. Names do not conflict because the base project is never loaded.

If I then ran

require 'tmpgem-base'
TmpGem::Tmp

The class originates from tmpgem-base. So Ruby won't throw an error for name conflicts, it will just overwrite any conflicting names.

[JakeUrban]

So @leighmcculloch I don't think we need to release a new version for ruby-stellar-base. We can keep it as-is so anyone using it without ruby-stellar-sdk is unaffected.

We can copy its contents into ruby-stellar-sdk, remove the ruby-stellar-base dependency and release a new version. Anyone who upgrades the SDK will likely not even notice anything has changed.

[leighmcculloch]

That sounds great. I think this works assuming a developer hasn't included both stellar-sdk and stellar-base in their gemspec or Gemfile. If they have, it won't matter that stellar-sdk has been updated and had the dependency removed from there, there will still be errors. In this case is probably fine though, the error will prompt them to remove the stellar-base dependency.

Do we know what that ☝️ error is, and if it will be clear enough what they should do to fix it?

We might be able to detect in stellar-sdk that stellar-base has been loaded and raise a very descriptive and clear error. We should be able to do that by checking that any type of value in base exists. @JakeUrban Thoughts? Is that something you could test in your tmpgem project?

[JakeUrban]

I think this works assuming a developer hasn't included both stellar-sdk and stellar-base in their gemspec or Gemfile. If they have, it won't matter that stellar-sdk has been updated and had the dependency removed from there, there will still be errors.

I think it pretty safe to assume they won't have both projects explicitly required, it'll be one or the other. If they do have both, and they require both in their code, then yes the two would conflict. But, it would do so silently like I showed in the last part of my example. Ruby doesn't throw an error, it just replaces the existing members that match the new members loaded.

We can notify the user by raising an error from stellar-sdk.rb, the top-level file for the project. I think that should be enough, considering they won't be able to get their project to work in this case.

[tomerweller]

I think it pretty safe to assume they won't have both projects explicitly required, it'll be one or the other.

@JakeUrban, I wouldn't be so sure about that. If someone is using base to directly compose non-trivial transactions, there's a good chance they're also using the sdk to submit these to horizon and/or utilize the sep support.

[JakeUrban]

Both projects use the same module. So when you require 'stellar-sdk', you get everything from stellar-base too, since its all in the Stellar:: module.

Anyone who has both projects require'ed must not be aware of that, which I think is unlikely given that the README and examples use code defined in stellar-base.

[leighmcculloch]

I don't think we should assume they aren't both explicitly required. Somebody is probably doing this. But we should help get people off base if they're continuing to use an old base with the new sdk that contains base. I think it's important we get people off it because within no time it will be out of date and incompatible with new XDR messages.

I'd favor a solution where if you input a new stellar-sdk with an old stellar-base we raise an error. It's preferable if that error is meaningful though and tells them exactly what's going on and what they need to do.

[nebolsin]

With astroband/ruby-stellar-sdk#86 we now have stellar-base merged into ruby-stellar-sdk repo in the protocol13 branch, we'll close this issue when it hits the master and the new versions of the gems released.

[charlie-wasp]

Well, the stellar-base was merged in the master branch of stellar-sdk 🎉 So I close this issue for now. Feel free to open a new one in ruby-stellar-sdk repo, if you have any issues or questions