Skip to content

refactor!: change chainId to enum#31

Merged
chybisov merged 24 commits into
mainfrom
ft-remove-get-chain-id
Aug 28, 2025
Merged

refactor!: change chainId to enum#31
chybisov merged 24 commits into
mainfrom
ft-remove-get-chain-id

Conversation

@tomiiide
Copy link
Copy Markdown
Contributor

@tomiiide tomiiide commented Aug 8, 2025

This PR:

  • closes https://lifi.atlassian.net/browse/LF-13925
  • removes invalid comments
  • refactors chain id to an enum
  • adds support for other bitcoin chains. (testnet, signet, fractal)
    • adds definition and sample test for signet
  • adds switchChain() and onChainChanged event handlers to supported wallets
    • Onekey
    • Unisat
    • Xverse
    • Binance wallet
    • Bitget

@tomiiide tomiiide marked this pull request as ready for review August 13, 2025 10:36
@tomiiide tomiiide self-assigned this Aug 13, 2025
@tomiiide tomiiide requested a review from chybisov August 13, 2025 10:36
rpcUrls: {
default: {
http: [
'https://rpc.ankr.com/btc_signet/c58ee1c627c5ad7cd69197c352aa51bdcebd87b86e0363c1e5133d5735cb3c69',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chybisov Couldn't find a public testnet rpc, does thorswap have a public rpc for signet or testnet?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find an rpc for testnet here, only comsos, and tendermint

@effie-ms
Copy link
Copy Markdown
Contributor

@tomiiide you might want to remove empty file packages/core/src/types/base.ts

@effie-ms effie-ms self-requested a review August 13, 2025 12:03
Comment thread packages/client/src/connectors/bitget.ts Outdated
Comment on lines +202 to +208
// debounced because xverse wallet calls the event handler twice in rapid succession
chainChange = debounce(
(event: XverseNetworkChangeEventParams) =>
this.onChainChanged(XverseBitcoinChainIdMap[event.bitcoin.name]),
300
)
provider.addListener('networkChange', chainChange)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What issues can it cause if used without debounce?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If a user is connected to his xverse wallet, and he changes network on the wallet, the connect dialog to the new network would pop up twice

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we sure 300ms is consistently enough?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm sure, but I'll test again


export const bitcoin = /*#__PURE__*/ defineChain({
id: 20000000000001,
id: ChainId.BITCOIN_MAINNET,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we sure this won't cause any trouble for LI.FI integration? 20000000000001 is used as an internal ID for the Bitcoin chain there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Need a small change required during the creation of the UXTO client.
lifinance/sdk#296

I've tested, and it didn't cause any trouble. Everything works fine.

Copy link
Copy Markdown
Member

@chybisov chybisov Aug 26, 2025

Choose a reason for hiding this comment

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

@tomiiide do we still need to pass chainIds here after we merge this https://github.com/lifinance/widget/blob/main/packages/wallet-management/src/createDefaultBigmiConfig.ts ? I guess not 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No we do not

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for confirming! Please make a PR to the widget to remove that once this is merged 🙏

@tomiiide
Copy link
Copy Markdown
Contributor Author

@tomiiide you might want to remove empty file packages/core/src/types/base.ts

this was removed in this already.

@tomiiide tomiiide requested a review from chybisov August 14, 2025 14:59
@tomiiide tomiiide requested a review from effie-ms August 14, 2025 14:59
Comment on lines +22 to +26
const { forward: OneKeyBitcoinNetworkChainIdMap, reverse: ChainIdToOneKeyMap } =
createBidirectionalMap<OneKeyBitcoinNetwork, ChainId>([
['livenet', ChainId.BITCOIN_MAINNET],
['testnet', ChainId.BITCOIN_TESTNET],
] as const)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we don't export this, can we move it to the connector creation function (line 74 here)? The same can be applied to other connectors.

Comment on lines +202 to +208
// debounced because xverse wallet calls the event handler twice in rapid succession
chainChange = debounce(
(event: XverseNetworkChangeEventParams) =>
this.onChainChanged(XverseBitcoinChainIdMap[event.bitcoin.name]),
300
)
provider.addListener('networkChange', chainChange)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we sure 300ms is consistently enough?

@tomiiide tomiiide requested a review from chybisov August 27, 2025 16:14
@chybisov chybisov changed the title refactor: change chainId to enum refactor!: change chainId to enum Aug 28, 2025
@chybisov chybisov merged commit 4e93517 into main Aug 28, 2025
@chybisov chybisov deleted the ft-remove-get-chain-id branch August 28, 2025 15:15
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