search

RootSoft / walletconnect-dart-sdk

Open protocol for connecting dApps to mobile wallets with QR code scanning or deep linking.
MIT License
102 stars 61 forks source link

chainId can be String #15

Open josh-burton opened 2 years ago

josh-burton commented 2 years ago

Currently chainId is an int, and fails if the chainId is returned as String. It is possible for the chainId to be a String so the param should be a String rather than int.

RootSoft commented 2 years ago

Thanks for the PR. I will review it asap

RootSoft commented 2 years ago

I will update the chainId type to a String when releasing the stable v1 release to not introduce any breaking changes (hence the reason I wanted to opt in for a dynamic chain id).

josh-burton commented 2 years ago

Since the package is pre 1.0 its probably a good time to make a breaking change, although it's a pretty minor breaking change as its relaxing a type restriction.

Waiting for 1.0 is probably going to mean this package doesn't work for many people.

RootSoft commented 2 years ago

I've did some more testing and it will be a more breaking change then expected. Different wallets return the chain id as ints or strings and might result in casting exceptions when parsing the json, so in the end a dynamic type checking must be performed anyways.

It might also break current existing storage solutions (e.g. wallet_connect_secure_storage) I don't see a problem with using a dynamic type field as the json_serializable package will handle the casting internally, and won't force people to change integer chain ids (which are most common) to strings. (https://chainlist.org/)

Kleak commented 1 year ago

any update on this ? we got the problem with Elrond wallet maiar