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

[Soroban] Add token parsing/formatting functions #106

Closed

Conversation

CassioMG
Copy link
Contributor

https://stellarorg.atlassian.net/jira/software/c/projects/WAL/boards/37?selectedIssue=WAL-1367

Differences when compared to Freighter's implementation:

  • Both functions take all 4 input types: bigint | BigNumber | number | string;
  • parseTokenAmount function returns bigint instead of BigNumber since bigint seems to be the default numbering format used in the soroban env;

@jeesunikim
Copy link

@CassioMG , js-stellar-base has these methods available (code). It looks like typescript imports js-stellar-sdk which imports js-stellar-base. Is it possible that this is a duplicate?

@CassioMG
Copy link
Contributor Author

@CassioMG , js-stellar-base has these methods available (code). It looks like typescript imports js-stellar-sdk which imports js-stellar-base. Is it possible that this is a duplicate?

@jeesunikim that's a very good point. @piyalbasu @aristidesstaffieri would you guys know the reason why we've created our own version of those functions in Freighter? Is this because of some type compatibility issue? Thanks!

@jeesunikim
Copy link

jeesunikim commented Mar 14, 2024

ah @CassioMG, my bad — I had this task [dApps] Move common formatters from Freighter/dApps to shared libraries that I never got to complete because I was waiting for some PRs to be merged :hide_homer:. I was told not to work on importing formatters for dApps since dApps required so dependencies updates and they weren't prioritized.

I can work on this for Freighter tomorrow.

@aristidesstaffieri
Copy link
Contributor

@CassioMG , js-stellar-base has these methods available (code). It looks like typescript imports js-stellar-sdk which imports js-stellar-base. Is it possible that this is a duplicate?

@jeesunikim that's a very good point. @piyalbasu @aristidesstaffieri would you guys know the reason why we've created our own version of those functions in Freighter? Is this because of some type compatibility issue? Thanks!

I think it's just that they existed in Freighter first and we haven't gotten around to using the ones in the sdk yet.

@Ifropc
Copy link
Contributor

Ifropc commented Mar 14, 2024

  1. I'd argue it's better to create another module for soroban tools, I'd assume there's going to be a big ask for it.
  2. We may want to move Soroban related stuff from Freighter to the wallet SDK (we don't need to do it at once, but we can start moving it bit by bit)
  3. On stuff that's already in stellar-base or horizon stellar SDK, it should be a fundamental functionality and we should not touch it

@CassioMG
Copy link
Contributor Author

  1. I'd argue it's better to create another module for soroban tools, I'd assume there's going to be a big ask for it.

@Ifropc do you mean creating a separate module like @stellar/typescript-wallet-sdk-soroban, similar to what we are doing for keymanager (@stellar/typescript-wallet-sdk-km)?

  1. We may want to move Soroban related stuff from Freighter to the wallet SDK (we don't need to do it at once, but we can start moving it bit by bit)

I like this idea

  1. On stuff that's already in stellar-base or horizon stellar SDK, it should be a fundamental functionality and we should not touch it

What do you mean by this exactly? I know it is advised to not import stellar-base directly in our code, but it should be safe to import stuff from stellar-sdk in case it is exposed there.
I just did a quick test here and it seems to be possible to import those functions from stellar-sdk. Like this:

import { Soroban } from "@stellar/stellar-sdk";

Soroban.formatTokenAmount("123456", 4);
Soroban.parseTokenAmount("12.3456", 4);

piyalbasu
piyalbasu previously approved these changes Mar 15, 2024
@piyalbasu piyalbasu dismissed their stale review March 15, 2024 17:11

Premature approval

@piyalbasu
Copy link
Contributor

Sorry for the confusion here - I didn't realize stellar-base implemented these methods. @aristidesstaffieri Is correct that our helpers pre-date the stellar-base methods.

If @Ifropc 's suggestion is to create a @stellar/typescript-wallet-sdk-soroban module, I tend to agree. The base @stellar/typescript-wallet-sdk stuff tends to be more Stellar Classic, so it might be good to split them

In reference to On stuff that's already in stellar-base or horizon stellar SDK, it should be a fundamental functionality and we should not touch it: I agree that we shouldn't re-do code that exists in stellar-sdk/stellar-base. I'd be in favor of just re-exporting the methods we want.

Another thought: is there any benefit to re-exporting all of stellar-sdk in this library? In this case, as a dev, I could just install @stellar/typescript-wallet-sdk and have access to all of the convenient methods of ts-wallet-sdk while still being able to grab anything from stellar-sdk/stellar-base in case I wanted to do something specific the wallet-sdk isn't configured to do yet 🤔 Just a thought.

@Ifropc
Copy link
Contributor

Ifropc commented Mar 15, 2024

On stuff that's already in stellar-base or horizon stellar SDK, it should be a fundamental functionality and we should not touch it

Oh my point is that if it's something that is already in base SDK we should just use that functionality instead (i.e. not reinventing the wheel) -> not related to this PR, as frankly I'm not familiar with Soroban codebase at all, but just in general thought for the future.
We suffer from not very clear boundaries of what should be where. Is it wallet SDK? Is it Horizon SDK?
IMO, we should define this boundaries early on. Stuff that's related to Soroban fundamentals (such as XDRs), should be in Horizon SDK. Stuff that's more high level (like working with contracts maybe?) should be in the wallet SDK.
Would also be great to hear @Shaptic 's thoughts on this

@acharb
Copy link
Contributor

acharb commented Mar 20, 2024

+1 for a @stellar/typescript-wallet-sdk-soroban package if the soroban code is gonna continue to grow

@CassioMG if you point the PR to release/1.4.0 it has the change of using a monorepo. And then the setup can be the same as typescript-wallet-sdk-km package

@CassioMG
Copy link
Contributor Author

CassioMG commented Mar 25, 2024

Hey guys, thanks for all the feedback and sorry about the late response - I was OOO last week

I'm planning on pointing my [Soroban] PRs to release/1.5.0 and create the @stellar/typescript-wallet-sdk-soroban package there.

As in regards to the token parsing/formatting functions I think I'll just re-export from stellar-sdk as you guys suggested.

Let me know in case you have further comments - thanks!

@Shaptic
Copy link

Shaptic commented Mar 25, 2024

Oh my point is that if it's something that is already in base SDK we should just use that functionality instead (i.e. not reinventing the wheel) -> not related to this PR, as frankly I'm not familiar with Soroban codebase at all, but just in general thought for the future.
We suffer from not very clear boundaries of what should be where. Is it wallet SDK? Is it Horizon SDK?
IMO, we should define this boundaries early on. Stuff that's related to Soroban fundamentals (such as XDRs), should be in Horizon SDK. Stuff that's more high level (like working with contracts maybe?) should be in the wallet SDK.
Would also be great to hear @Shaptic 's thoughts on this

Thanks for the ping! For the specific case of the formatting and parsing functions, those were a feature added by @jeesunikim that felt useful and fundamental enough to merge into stellar-base (see stellar/js-stellar-base#667). Was that premature? Perhaps - we had no well-defined boundaries as @Ifropc says. But, as Chinese wisdom states: "The best time to plant a tree was 20 years ago. The second best time is now."

I've discussed this a lot with folks like @piyalbasu and @chadoh in the context of the TypeScript bindings in the Soroban CLI as well as the related generated ContractClient for interacting with contracts (see e.g. stellar/js-stellar-sdk#891 (review), though a lot of the discussion was done synchronously). The general consensus we came to in this specific context was (correct me if I'm wrong, fellas): anything in the SDK should aim to be as generic and helpful as possible, especially avoiding anything wallet-related or wallet-specific, and it should compile out of the final bundle when people aren't using it.

However, I was wary of this because of a similar fear as @Ifropc:

I'd argue it's better to create another module for soroban tools, I'd assume there's going to be a big ask for it.

I think it would be very valuable to have a plug-and-play layer above the SDK with high-level, Soroban-focused functionality. I don't have strong opinions on its naming or functionality, but to me it does seem very clear that there is a layer between the stellar-sdk (which, traditionally, has been XDR and HTTP wrappers) and wallets (which do a LOT) that is missing for Soroban. Formatters, generated clients, etc. (and, arguably, even things like scValToNative) feel like they may belong in this layer. Anyone building on Soroban will likely need these extra features, but anyone who doesn't need it will not.

+1 for a @stellar/typescript-wallet-sdk-soroban package if the soroban code is gonna continue to grow

Would there be value in just a ts-soroban-sdk-like package, or are there truly wallet-specific features that we feel would belong in this package? While I do think most people will interact with Soroban through browser-powered dApps that leverage wallets, it's important to keep in mind non-browser environments, too (e.g. backends, CLI tools, or even self-managed front-ends that do their own secret management).

@Ifropc
Copy link
Contributor

Ifropc commented Mar 26, 2024

Just as a heads-up, even though we named the SDK "wallet-sdk" even now it extends in usage above just the wallet integrations. We still in a "figuring out" stage of what the future ask is, but even now the wallet SDK can (and should) be used on the backend too.
So I think it makes sense to have a monorepo for "client apps" (anything that connects to Stellar network) SDK (which right now we simply name Wallet SDK, but maybe we can rebrand it in the future), which includes obviously wallets, but also backend, browser applications and any JS tooling

@CassioMG
Copy link
Contributor Author

Hey everyone, I'm closing this PR in favor of #120 which adds the token parsing/formatting functions to the new soroban workspace. As you can check there it is now re-exporting the existing functions from stellar-base. Please feel free to add any comments there as well, thanks!

@CassioMG CassioMG closed this Mar 28, 2024
@CassioMG CassioMG deleted the cg-soroban-format-amount branch March 28, 2024 23:19
This pull request was closed.
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.

7 participants