Refactor/sp address use raw bytes#137
Conversation
Sosthene00
force-pushed
the
refactor/sp_address_use_raw_bytes
branch
3 times, most recently
from
53e3038 to
8cfa0a5
Compare
|
Hello @Sosthene00 |
|
Hi @sdmg15 and thanks for reaching out! #143 was a bit big and hard to review so we splitted it in smaller PRs, that's why you will find that changes in this commit and some others are redundant with #143 I will close it now to prevent further confusion. Regarding the changes made in that PR specifically, while it can be reviewed and tested in isolation (that was the whole point of splitting everything in smaller PRs in the first place), it has more sense considered with #138 #140 #141 #142 I'm not quite done moving every changes I made for psbts on that branch, so more PRs (1 or 2) are coming, I'll try to do that this week so that you can have the whole picture. |
| /// * Edge cases are hit during elliptic curve computation (extremely unlikely). | ||
| pub fn generate_recipient_pubkeys( | ||
| recipients: Vec<SilentPaymentAddress>, | ||
| recipients: Vec<SilentPaymentAddressRaw>, |
There was a problem hiding this comment.
Maybe instead of having recipients: Vec<SilentPaymentAddressRaw>, take recipients: impl Into<Vec<SilentPaymentAddressRaw>>, then we implement From<SilentPaymentAddress> for SilentPaymentAddressRaw to convert them automatically (which can be done in this direction). Then this function can take both Vec<SilentPaymentAddress> and Vec<SilentPaymentAddressRaw>.
Then we also don't need the from_byte_array and to_byte_array functions.
There was a problem hiding this comment.
That's elegant, but maybe a bit overkill here, since in practice generate_recipient_pubkeys is used inside higher level methods and will arguably mostly be used this way by downstream consumers of the lib. What I mean is that it's mostly about moving a simple conversion that happens in some place of our code higher in the stack.
| .expect("We did the same check than SilentPaymentAddress") | ||
| } | ||
|
|
||
| pub fn get_scan_pubkey(&self) -> PublicKey { |
There was a problem hiding this comment.
If we're going to implement these high-level accessor functions then I think we might as well just define SilentPaymentAddressRaw as {version: SpVersion, scan_pubkey: PublicKey, m_pubkey: PublicKey }.
There was a problem hiding this comment.
I don't know, the point of that type was to be a very thin wrapper around a bytes array, since it turned out it was easier and faster to work with for some methods. The PublicKey accessors make sense as a convenience, but the point is the other methods that let us read slice of raw bytes
There was a problem hiding this comment.
Since we're actively interpreting the contents of the byte array (reading both P_scan and P_m in generate_recipient_pubkeys), I don't think the 'thin wrapper' approach makes sense after all.
The only difference in how we use SilentPaymentAddress vs SilentPaymentAddressRaw is that the former contains knowledge of the intended network. That's useful in 2 cases:
- when parsing an address string, to check if the network of the parsed address is correct
- displaying the address, which requires the network type
Both of those cases are user/UI-facing. So I think the more sensible approach is to make the SilentPaymentAddress be the 'thin' struct for display/input parsing purposes, and make SilentPaymentAddressRaw be the 'main' struct for all the sp logic. What do you think of that approach?
There was a problem hiding this comment.
I gave it a quick shot, that's pretty straightforward and it works, not sure of the implication, probably not much difference beside it being slightly more readable so that's still a win I guess. I gotta go will push a new version asap
…e address methods Extract network-agnostic address data into a new SilentPaymentAddressRaw type. Rename get_scan_key/get_spend_key/get_network/get_version to scan_key/spend_key/network/version, and change version() to return SpVersion instead of u8. Add byte serialization helpers, new_v0/from_raw constructors, and From conversions between the two types. Co-authored-by: Cursor <cursoragent@cursor.com>
…::new Co-authored-by: Cursor <cursoragent@cursor.com>
generate_recipient_pubkeys now accepts and returns SilentPaymentAddressRaw, removing the network information from the type used in the actual ECC computation. Co-authored-by: Cursor <cursoragent@cursor.com>
Mechanically replace get_scan_key/get_spend_key/get_network/get_version with the renamed scan_key/spend_key/network/version across examples, tests, and spdk-wallet. Also convert SilentPaymentAddress to SilentPaymentAddressRaw where the sending API is called. Co-authored-by: Cursor <cursoragent@cursor.com>
Sosthene00
force-pushed
the
refactor/sp_address_use_raw_bytes
branch
from
8cfa0a5 to
a482803
Compare
3 participants
Acknowledging that we often can't use
SilentPaymentAddresstype because we don't have the network, or we can have it but it's irrelevant, especially in psbt context, we need a type that only contains version, scan key and spend key. We also need to do back and forth with a bytes array representation of those data.Instead of stripping the existing type of
network, which would break all existing code, we propose to define another, simpler type,SilentPaymentAddressRaw, that is basically a very thin wrapper around a[u8; 67], but also offers type guarantees and more readability.First place we would use that type is
generate_recipient_pubkeys