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.

Sign up for GitHub

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

Jump to bottom

Fixes type mismatches between in RN & iOS libs #409

Open
wants to merge 2 commits into
base: np/smart-contract-wallets
Choose a base branch
from
Open

Fixes type mismatches between in RN & iOS libs #409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
154ff6d
match ios sca types to RN
peterferguson Oct 21, 2024
a4ff78e
fix unwrap chain id on ios createV3
peterferguson Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Sources/XMTPiOS/Client.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@peterferguson peterferguson Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"eip155:\(String(describing: account.chainId!)):\(account.address.lowercased())" :
account.address
let inboxId = try await getOrCreateInboxId(options: options, address: accountAddress)

Expand All @@ -210,9 +210,9 @@ public final class Client {
)
}

public static func buildV3(address: String, chainId: Int64? = nil, options: ClientOptions) async throws -> Client {
public static func buildV3(address: String, chainId: UInt64? = nil, options: ClientOptions) async throws -> Client {
let accountAddress = chainId != nil ?
"eip155:\(String(describing: chainId)):\(address.lowercased())" :
"eip155:\(String(describing: chainId!)):\(address.lowercased())" :
address
let inboxId = try await getOrCreateInboxId(options: options, address: accountAddress)

Expand Down
8 changes: 4 additions & 4 deletions Sources/XMTPiOS/SigningKey.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍


/// The blockNumber of the chain for example "1"
var blockNumber: Int64? { get }
var blockNumber: UInt64? { get }

/// Sign the data and return a secp256k1 compact recoverable signature.
func sign(_ data: Data) async throws -> Signature
Expand All @@ -45,11 +45,11 @@ extension SigningKey {
return false
}

public var chainId: Int64? {
public var chainId: UInt64? {
return nil
}

public var blockNumber: Int64? {
public var blockNumber: UInt64? {
return nil
}

Expand Down
Loading