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

Add additional wallets to a client #383

Closed
wants to merge 10 commits into from

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Aug 8, 2024

This adds the ability to add additional wallets to an existing client.

Client creation only allows creating with an EOA so we can guarantee V2 functionality works as intended. However you can add an additional EOA or SCW.

On initial client creation a chainRPC needs to be set and on all additional SCW added they need to conform to CAIP addressing https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-10.md

@nplasterer nplasterer self-assigned this Aug 8, 2024
@@ -19,6 +19,9 @@ import LibXMTP
public protocol SigningKey {
/// A wallet address for this key
var address: String { get }

/// Chain rpc url for the smart contract wallet
var chainRPCUrl: String? { get }
Copy link
Contributor Author

@nplasterer nplasterer Aug 8, 2024

Choose a reason for hiding this comment

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

@neekolas I think I need some knowledge sharing on how this chainRPC is supposed to be set. If it's okay for it to be on the signing key which is an interface implemented by the integrator.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was all done a while ago, and not by me, so my recollection is a little fuzzy on where and when we need to pass in the chainRPC.

But generally it's a thing that dapps will have (it's the URL you'd get from an Infura or Alchemy dashboard). It wouldn't be "key specific", so it's probably safe to make it a client create parameter and save it there. We'd use the same RPC url for all signing keys.

We need it at the time we are creating or verifying smart contract wallet signatures.

@@ -602,4 +602,26 @@ public final class Client {
}
try await client.requestHistorySync()
}

public func addWallet(account: SigningKey) async throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After adding the wallet to their identity what benefits do they get/ how can the interact with the wallet?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would allow someone else to enter any one of the added wallet addresses and reach the same inbox.

And for an app that has access to any of the added wallets to create a new installation for that inbox.

@nplasterer nplasterer marked this pull request as ready for review August 20, 2024 02:44
@nplasterer nplasterer requested a review from a team as a code owner August 20, 2024 02:44

let alixWallet2 = try PrivateKey.generate()
try await alix.addWallet(account: alixWallet2)
XCTAssertEqual(alix.addresses.count, 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neekolas what would be a good thing to test on a client to see if a wallet was added? Is there something I can expose from libxmtp to show me all the wallets attached?

Copy link
Contributor

Choose a reason for hiding this comment

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

The one we already expose is group.members() which will show all the account addresses for each member

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can for sure use that in the test. But what if I want to see all the addresses I have on my account so I can revoke some? Should we also expose addresses on client?

Copy link
Contributor

@neekolas neekolas Aug 20, 2024

Choose a reason for hiding this comment

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

Easy enough to add. We have all the information in the local DB. Happy to pick that one up, should be a small task.

Would definitely be helpful for revocation, where you need to see all your own account addresses and installations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we talk about revoking a wallet that is different than revoking installations correct? Revoking a wallet would just revoke all the installations for that address?


public struct FakeSCWWallet: SigningKey {
public static func generate() throws -> FakeWallet {
let key = try PrivateKey.generate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there is something simple like this for generating a test SCW... seems unlikely. But I can atleast test the account format and isSmartContractWallet.

@@ -19,6 +19,9 @@ import LibXMTP
public protocol SigningKey {
/// A wallet address for this key
var address: String { get }

/// If this signing key is a smart contract wallet
var isSmartContractWallet: Bool { get }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also pass a chainId

@nplasterer nplasterer closed this Dec 20, 2024
@nplasterer nplasterer deleted the np/add-smart-contract-wallet branch December 20, 2024 22:22
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.

2 participants