Conversation
This comment has been minimized.
This comment has been minimized.
ben-kaufman
force-pushed
the
feat/multiple-addresses-types
branch
from
919e450 to
8c8e243
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@claude create a checklist of manual test flows |
This comment has been minimized.
This comment has been minimized.
ben-kaufman
force-pushed
the
feat/multiple-addresses-types
branch
from
1170e77 to
b0e40c3
Compare
There was a problem hiding this comment.
Test 1
Ran successfully the 1st pass of tests: switching and receiving to each address type as configurable in settings.
@ben-kaufman
I noticed the Address Viewer only knows the native segwit address type. Maybe worth updating it with implementation matching the former react native functionality which allowed switching address type on that screen. Probably best done on another PR to keep the changes reviewable; can be stacked PR, targeting the branch of this one.
|
@ovitrif I just updated the address viewer here since it was a very small change |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Didn't find any actual bug, just some suggestions
Still testing...
| private static func parseAddressType(_ string: String?) -> LDKNode.AddressType { | ||
| switch string { | ||
| case "legacy": return .legacy | ||
| case "nestedSegwit": return .nestedSegwit | ||
| case "taproot": return .taproot | ||
| default: return .nativeSegwit | ||
| } |
There was a problem hiding this comment.
| do { | ||
| let newAddress = try await lightningService.newAddressForType(addressType) | ||
|
|
||
| UserDefaults.standard.set(newAddress, forKey: "onchainAddress") |
There was a problem hiding this comment.
nit could check defensively if the address matches the addressType before saving it and even add unit tests
ben-kaufman
force-pushed
the
feat/multiple-addresses-types
branch
from
21fca73 to
dc1ab2a
Compare
|
@jvsena42 updated for all comments |
Co-authored-by: João Victor Sena <jvsena42@users.noreply.github.com>
|
I didn't know that Claude would automatically commit 😶🌫️ |
| currentWalletAddress: String, | ||
| selectedAddressType: LDKNode.AddressType | ||
| ) async throws -> String? { | ||
| guard !isSearching else { return nil } |
There was a problem hiding this comment.
could enqueue instead of returning nil, to reduce the risk of returning a wrong address in this fallback, or at least log this with a warning
There was a problem hiding this comment.
but enqueue could also make the sync slower
| let monitoredTypes = monitoredTypesString.split(separator: ",") | ||
| .map { String($0).trimmingCharacters(in: .whitespaces) } | ||
| .compactMap { LDKNode.AddressType.from(string: $0) } |
There was a problem hiding this comment.
this logic is also duplicated, could move it to LDKNode.AddressType
3 participants
This PR adds support for multiple Bitcoin address types in the iOS app, allowing users to select and monitor different address formats. Users can select their preferred address type from: Legacy (P2PKH), Nested SegWit (P2SH-P2WPKH), Native SegWit (P2WPKH), and Taproot (P2TR).
This requires extensive testing across all functionalities, send, receive, channel open, channel close (funds should always go to native segwit address except when selected address when channel opened was Taproot, in which case it should be a Taproot address), cpfp, rbf. All should be checked for each address type, and for multiple address types combined (ie. send with utxos of different address types, open channel with inputs of different address types, boost txs with inputs of different address types, etc.)