Fixes type mismatches between in RN & iOS libs #409
Conversation
| @@ -24,10 +24,10 @@ public protocol SigningKey { | |||
| var isSmartContractWallet: Bool { get } | |||
|
|
|||
| /// The name of the chainId for example "1" | |||
| var chainId: Int64? { get } | |||
| var chainId: UInt64? { get } | |||
There was a problem hiding this comment.
I actually think we should keep these Int64 and change the RN to Int64. I do all the U things to it in the code already so no need for these to be UInt64 here.
There was a problem hiding this comment.
Yea I changed this here for two reasons
- Passing the ReactNativeSigner as a SigningKey doesn't work as the chainId getter have different types. So needed to align those.
- When aligning I choose to stick with the eip155 definition of chainId as unsigned
I don't think it matters that much either way so whatever works 👍
| @@ -197,8 +197,8 @@ public final class Client { | |||
| } | |||
|
|
|||
| public static func createV3(account: SigningKey, options: ClientOptions) async throws -> Client { | |||
| let accountAddress = account.isSmartContractWallet ? | |||
| "eip155:\(String(describing: account.chainId)):\(account.address.lowercased())" : | |||
| let accountAddress = account.chainId != nil ? | |||
There was a problem hiding this comment.
Do you not set the boolean for isSmartContractWallet? Do you only set the chainId? Wondering how we can make it more clear that the both need to be set if you're using a smart contract wallet. Though I guess we could get away with removing isSmartContractWallet potentially if you think the chainId will be enough? Or for ease of feature advancements we could change it to a type EOA or SCW etc...
There was a problem hiding this comment.
I think chainId is enough but I am not sure if dropping the isSmartContract leads to the clearest api. I can see people just seeing that the chainId field can be entered and passing it even with EOAs.
For RN I was thinking maybe an object might work but don't think that carries over to native so well.
Could adopt the chain-specific address as it has become relatively common among SCA providers. So those including an address field of the form {chainId or chainShortName}:{address} get setup as SCA — although maybe this is less clear.
I wasn't actually judging the api though. I changed this is because the chainId needs to be unwrapped here & I just went with the condition from the buildV3 method.
Introduction 📟
Purpose ℹ️
Scope 🔭