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

“Add Token” via freighter-api #1815

Open
wants to merge 29 commits into
base: release/5.28.0
Choose a base branch
from
Open

Conversation

CassioMG
Copy link
Contributor

@CassioMG CassioMG commented Jan 25, 2025

Related issue: #1646

This PR adds the addToken functionality via freighter-api, it uses the existing signTransaction and signMessage APIs as inspiration.

The addToken API takes both a contractId and a networkPassphrase as input which Freighter will use to load the token details (e.g. symbol, name, decimals and balance) for the user.

Freighter will then show these token details along with any applicable warnings. The user will need to approve it in order for the token to be added.

This PR extracts the token lookup and change trustline functionalities into the useTokenLookup and useChangeTrustline hooks respectively so we can reuse the "add token" logic in both our API and manual approaches.

Adding a custom token with the addToken API

custom-token.mov

Adding a SAC token with the addToken API

sac-token.mov

Asset lists and Blockaid warning messages

Screenshot 2025-02-03 at 16 34 53 Screenshot 2025-02-03 at 16 21 31 Screenshot 2025-02-03 at 16 18 50 Screenshot 2025-02-03 at 16 22 57 Screenshot 2025-02-03 at 16 23 24

Documentation update

Screenshot 2025-02-03 at 17 50 10 Screenshot 2025-02-03 at 17 50 28

@CassioMG CassioMG changed the title [DRAFT] “Add Token” via freighter-api “Add Token” via freighter-api Feb 4, 2025
@CassioMG CassioMG linked an issue Feb 4, 2025 that may be closed by this pull request
@CassioMG CassioMG marked this pull request as ready for review February 4, 2025 00:49
try {
let balance;
if (fetchBalance && networkDetails.sorobanRpcUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason to not use the balance from the backend in this case? We only want to use the rpc locally in the case of a custom network in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

Second this. We should use the backend to fetch balances and token details (unless we're on a custom network).

Also, using buildSorobanServer with networkDetails.sorobanRpcUrl will not work on mainnet because our Soroban RPC url for Mainnet is on a private kube cluster that only our backend can reach. You'll see that if you hit this method on Mainnet that the call will fail

Copy link
Contributor Author

@CassioMG CassioMG Feb 5, 2025

Choose a reason for hiding this comment

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

@aristidesstaffieri @piyalbasu Thanks for raising this!

So I ended up adding this RPC fetch because the backend request was not returning the balance. It's currently returning an object like this:

{
  name: string;
  decimals: number;
  symbol: string;
}

I'll take a look at the backend code to see what we could do there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

>
+1000.00 GO
{t("Simulated Balance Changes")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this says "Simulated Balance Changes"?

Isn't adding a token only bringing into your Freighter balances? I don't think this changes a balance or simulates a balance change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I was simply following the Designs here. Looks like @sdfcharles tried using the same wording from other screens, but thinking about this now I think something like "Balance Info" would probably make more sense here.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah something like that seems more appropriate to me but maybe we could have @sdfcharles weigh in.

Copy link
Contributor Author

@CassioMG CassioMG Feb 6, 2025

Choose a reason for hiding this comment

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

@piyalbasu
Copy link
Contributor

I'm still taking a look at this (sorry for the delay!). But could you please add documentation for this new method in docs/docs/guide/usingFreighterWebApp.md? It'd be great to have this outlined next to all the other methods.

@CassioMG
Copy link
Contributor Author

CassioMG commented Feb 5, 2025

I'm still taking a look at this (sorry for the delay!). But could you please add documentation for this new method in docs/docs/guide/usingFreighterWebApp.md? It'd be great to have this outlined next to all the other methods.

@piyalbasu great catch, I'll add it

@CassioMG
Copy link
Contributor Author

CassioMG commented Feb 6, 2025

@piyalbasu I've added the missing documentation for addToken and I noticed it was also missing for the getNetworkDetails api so I went ahead and added both (here and here). Let me know what you think:

Screenshot 2025-02-06 at 15 25 36 Screenshot 2025-02-06 at 15 25 09

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.

“Add Token” via freighter-api
3 participants