Skip to content
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

Merge the Soroban RPC client into this library #860

Merged
merged 30 commits into from
Oct 23, 2023
Merged

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Sep 15, 2023

What

This merges the soroban-client library (stellar/js-soroban-client) into this package.

The namespaces have changed to move each server-dependent component into its own module. There is a detailed migration guide written here which should outline both the literal (i.e. necessary code changes) and philosophical (i.e. how to find certain functionality) changes needed to adapt to this merge.

The code changes themselves are:

  • moving js-soroban-client into src/soroban and its tests into test/unit/server/soroban
  • dropping duplicate files (like axios, etc.)
  • migrating imports to remove conflicts and make them sensible
  • moving namespaces around to separate Horizon and Soroban RPC in a sensible way
  • updating tests to work with both libs

Why

It is incredibly likely that most applications will need to interface with both Horizon and Soroban if they plan on supporting the entire feature set that Stellar has to offer. Putting things into a single library means less duplicated dependencies and a better, all-inclusive experience. Additionally, the amount of shared code and build dependencies across these two libraries means it makes sense to combine them into one.

Closes #850.

Known Limitations

  • If someone does not care about one server or the other, they may be blowing up their bundle size needlessly.
  • It may be better to have a monorepo structure to share build scripts rather than actually combining both libraries.
  • User feedback is non-existent, yet essential for determining whether or not this is worthwhile

TODOs Remaining

  • Write a migration guide for both Horizon and Soroban RPC users

@socket-security
Copy link

socket-security bot commented Sep 15, 2023

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Size Change: +847 kB (+8%) 🔍

Total Size: 12.1 MB

Filename Size Change
dist/stellar-sdk.js 6.96 MB +562 kB (+9%) 🔍
dist/stellar-sdk.min.js 5.1 MB +285 kB (+6%) 🔍

compressed-size-action

@Shaptic Shaptic changed the title [Draft] Merge the soroban-client library into this one Merge the Soroban RPC client into this library Sep 22, 2023
@Shaptic Shaptic marked this pull request as ready for review September 22, 2023 19:02
src/soroban/friendbot.ts Outdated Show resolved Hide resolved
test/unit/server_test.js Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Shaptic Shaptic requested a review from sreuland October 13, 2023 18:34
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work, getting this all wrangled together, I didn't review most of the copied soroban-client code, just focused on the exports, module layouts, and left a couple comments for consideration.

@Shaptic Shaptic merged commit 978eda3 into master Oct 23, 2023
7 checks passed
@Shaptic Shaptic deleted the soroban-merge branch October 23, 2023 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine Horizon and Soroban JS SDKs into a single repository
2 participants