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

fix: improve TON signer and MyTonWallet provider #914

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Ikari-Shinji-re
Copy link
Member

@Ikari-Shinji-re Ikari-Shinji-re commented Oct 21, 2024

Summary

  • Fix the issue where TON tokens are not being displayed correctly in the token list
  • Return the non-bounceable address for the MytonWallet provider
  • Retrieve and return the hash of the message from the BOC after signing the TON transaction
  • Add an option to retrieve the TON Connect manifest from the widget configuration.

How did you test this change?

  • Review the list of TON tokens in the token list. Ensure that if a token has an address or name, it is displayed correctly. You can verify this by checking the meta response from the network tab and comparing it with the UI.
  • Connect the MytonWallet provider. The address displayed in the confirmation wallet modal must match the address selected and shown in the MytonWallet UI.
  • Create a transaction using the TON blockchain as the source. Sign the transaction and check the network tab to ensure that the hash of the message is sent to the server with the checkStatus request.

Note

There are some considerations for using the provider API instead of the tonconnect/sdk. Since the main pull request was made over a year ago, I might not recall everything, but here are a few points:

  • I was concerned about creating multiple instances of the TON Connect SDK for each TON Connect-compatible provider we integrated.
  • The tonconnect/sdk uses callbacks to handle wallet connection events, which added extra effort for implementation.
  • Using the SDK will introduce an additional dependency and increase the bundle size, with relatively little benefit in return.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Implemented a user interface (UI) change, referencing our Figma design to ensure pixel-perfect precision.

@Ikari-Shinji-re Ikari-Shinji-re changed the title !WIP: fix: improve TON signer and MyTonWallet provider fix: improve TON signer and MyTonWallet provider Nov 20, 2024
@Ikari-Shinji-re Ikari-Shinji-re marked this pull request as ready for review November 20, 2024 07:09
@Ikari-Shinji-re Ikari-Shinji-re force-pushed the fix/improve-ton-signer-and-mytonwallet-provider branch from 901efcb to 17ad1fa Compare November 20, 2024 07:11
Comment on lines 15 to 16
export const TON_CONNECT_MANIFEST_URL =
'https://raw.githubusercontent.com/rango-exchange/rango-types/main/assets/manifests/tonconnect-manifest.json';
Copy link
Member

Choose a reason for hiding this comment

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

Please move default manifest to assets repo instead.

Copy link
Collaborator

@yeager-eren yeager-eren left a comment

Choose a reason for hiding this comment

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

Thanks for your effort on this. I've put some comments to address.
Please also make sure you will let QA know to check playground and exporting config as well.

widget/app/src/constants.ts Show resolved Hide resolved
widget/embedded/src/types/config.ts Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@
},
"browserslist": "> 0.5%, last 2 versions, not dead",
"scripts": {
"dev": "parcel -p 3002 --cache-dir=.parcel-cache --no-cache",
"dev": "parcel -p 3000 --cache-dir=.parcel-cache --no-cache",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this needed to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any changes to the API base URL, port, or API key are necessary for testing the staging environment. However, these changes must be reverted before merging this pull request.

wallets/provider-mytonwallet/src/types.ts Outdated Show resolved Hide resolved
widget/embedded/src/types/config.ts Outdated Show resolved Hide resolved
wallets/provider-all/src/index.ts Outdated Show resolved Hide resolved
wallets/provider-mytonwallet/package.json Show resolved Hide resolved
export function myTonWallet() {
return window.mytonwallet?.tonconnect ?? null;
}

export function parseAddress(rawAddress: string): string {
return Address.parse(rawAddress).toString({ bounceable: false });
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it is a backend requirement but can explain why it should be non-bouncable? for the record.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's not exactly the reason. I checked with the backend team, and they confirmed it doesn't make a difference for them. However, based on the documentation, I updated the address to a non-bounceable format. Additionally, if you connect using the MyTonWallet provider, you'll notice that the address shown in the provider extension now matches the one displayed in our app.
https://docs.ton.org/v3/documentation/smart-contracts/addresses#bounceable-vs-non-bounceable-addresses

widget/embedded/src/components/TokenList/TokenList.tsx Outdated Show resolved Hide resolved
@yeager-eren

This comment was marked as resolved.

Copy link
Collaborator

@yeager-eren yeager-eren left a comment

Choose a reason for hiding this comment

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

lgtm

@Ikari-Shinji-re Ikari-Shinji-re force-pushed the fix/improve-ton-signer-and-mytonwallet-provider branch from 3730062 to 575d599 Compare November 27, 2024 15:03
@yeager-eren yeager-eren merged commit 7027755 into next Nov 27, 2024
1 check failed
@yeager-eren yeager-eren deleted the fix/improve-ton-signer-and-mytonwallet-provider branch November 27, 2024 15:04
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.

3 participants