Enable retrieval of ephemeral addresses in zcashlc_list_transparent_receivers.
#160
Conversation
| @@ -862,29 +862,47 @@ pub unsafe extern "C" fn zcashlc_list_transparent_receivers( | |||
| db_data_len: usize, | |||
| account_id: i32, | |||
| network_id: u32, | |||
| include_ephemeral: bool, | |||
There was a problem hiding this comment.
This is a breaking change to zcashlc_list_transparent_receivers, so it cannot be merged directly to main (and risk being included in whatever the next release ends up being) and instead needs to go to a feature branch for the next FFI breaking release (0.11.0).
If this was instead a new FFI method then it could be in an 0.10.3 release.
There was a problem hiding this comment.
If it were only that then I'd have no strong opinion, but this design also makes it more difficult to tell which addresses are ephemeral and which are not. That's important because we might want to treat the latter differently wrt latency or privacy of queries. So I think another FFI method is the right thing here.
There was a problem hiding this comment.
Also this doesn't expose the index_range parameter of get_known_ephemeral_addresses. If it were a separate method then it would be natural to do that.
At the C API level, it's sufficient to represent Option<Range<u32>> as a pair of non-optional uint32_t parameters, because 0, uint32_t(1) << 31 is defined (and implemented for WalletDb in zcash_client_sqlite) to be equivalent to the None case for this API. This can be invisible to SDK users since the SDK can handle the mapping from a Swift Optional<Range<UInt32>>.
There was a problem hiding this comment.
The way I've been approaching this is that breaking changes go to main, semver-compatible changes go to a hotfix/ branch. I thought that's what we had agreed upon?
There was a problem hiding this comment.
We had agreed upon that for regular repositories, but this repository is unique in its committed binary requirement. If we merge breaking changes directly to main, then we have the problem that main has a split view: its code does not match its binaries, and it's the latter that actually get used.
If we want to have a primary development target branch rather than creating a separate feature branch for each release like we've done in the past, then we should do so explicitly:
- Have a
devbranch that is explicitly a split view branch, where the binary is never updated. - We cut breaking releases by making a branch from
dev, building the binaries, and merging that branch intomainvia a PR. - For testing the current state of the
devbranch, we could have adev-previewbranch that is maybe built by CI (if performant enough) that is force-pushed every timedevchanges (sodev-previewonly ever contains a single commit on top of the current state ofdev, ensuring that old preview binaries can get GCed).
There was a problem hiding this comment.
Okay, that makes sense, I had forgotten about the API/binary split issue. We should have a feature branch both for a semver-compatible hotfix, and also for the next semver-breaking release, and we should target each PR to one or both respectively. #158 and this PR should both be retargeted (and #158 can have a separate PR opened to the hotfix branch, if you prefer.)
We can't actually build the preview in CI because we have to build on XCode 13 to meet our iOS version compatibility requirements in order to avoid breaking Edge's process, and I don't think we can get a MacOS runner that is on an old enough version to still run XCode 13?
There was a problem hiding this comment.
The macos-12 runner is apparently still available (though deprecated), and supports as early as Xcode 13.1 (though IDK if minor revisions of Xcode matter in this instance). So I think we can at least for a while use CI for this.
| ) | ||
| })?; | ||
|
|
||
| keys.chain(ephemeral_keys.iter().map(|(addr, _)| { |
There was a problem hiding this comment.
If we're going to merge the two into the same API, then map |(addr, _)| addr and then share the FFIEncodedKey-producing closure instead of duplicating it.
@daira can you expand on why you think it should be a separate FFI method? IMO that just increases complexity for consumers. This method is specifically intended to return addresses that the app needs to check to find UTXOs belonging to the wallet, and in that respect there is no difference between the transparent addresses that the wallet has generated; remember that these are effectively external addresses: counterparties may use them to return funds to the wallet, so they are "public" information. The only reason that we generated them using a separate key path is we wanted to ensure that they would not collide with transparent addresses generated from a bitcoin-derived wallet. Furthermore, users of the SDK should not have to know about ephemeral addresses in general; we should not create yet another logical category of addresses that wallet developers need to consider when evaluating how to protect users' privacy. We want to avoid linking transparent addresses to one another, irrespective of whether those addresses are provided by the user to a counterparty directly, or indirectly as the consequence of a ZIP 320 transfer. |
That's irrelevant to the FFI interface, which is internal to the SDK. It's fine to have a separate method here, that is used alongside the existing method inside the SDK without exposing it. |
What is the advantage to having two separate methods here? |
No description provided.