Restrict use of backend-specific note identifiers. #888
Conversation
nuttycom
force-pushed
the
feature/tx_output_note_refs
branch
from
5ed49a4
to
bede07d
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
+ Coverage 70.89% 71.01% +0.11%
==========================================
Files 131 131
Lines 12656 12658 +2
==========================================
+ Hits 8973 8989 +16
+ Misses 3683 3669 -14
☔ View full report in Codecov by Sentry. |
| @@ -441,7 +440,7 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>( | |||
| min_confirmations: NonZeroU32, | |||
| change_memo: Option<MemoBytes>, | |||
| ) -> Result< | |||
| DbT::TxRef, | |||
| TxId, | |||
There was a problem hiding this comment.
[Non - Blocking] Although small, this is a great improvement!
I'm mostly sure it will break the ffi layer so it will be good to track this hiccup in the ffi repository on iOS and the android sdk r
nuttycom
force-pushed
the
feature/tx_output_note_refs
branch
4 times, most recently
from
0181c95
to
ef0b27c
Compare
| ReceivedNoteId(i64), | ||
| } | ||
|
|
||
| impl fmt::Display for NoteId { |
There was a problem hiding this comment.
Oops, I missed that the NoteId generic parameter on Error was actually just renamed to NoteRef, so we did still need a Display impl. Fixed in #890.
| let sent_memo = wallet::get_sent_memo(self.conn.borrow(), note_id)?; | ||
| if sent_memo.is_some() { | ||
| Ok(sent_memo) | ||
| } else { | ||
| wallet::get_received_memo(self.conn.borrow(), note_id) | ||
| } |
There was a problem hiding this comment.
This method is now concretely less efficient than before. I don't know by how much, but it is now O(num_received + num_sent + 2*num_internal), because we have two separate table joins against transactions (instead of a direct query of an indexed column), and for received notes we always query the sent notes table first. We'll know more once we get this into the mobile SDKs, so this is non-blocking, but we should track its effect on the history-rendering app views.
More non-blocking thoughts: Could we gain back or amortize some of the performance loss by combining the new queries into a single query against all three tables at once? AFAICT this is the only place that wallet::get_sent_memo and wallet::get_received_memo are used (they were removed from the public API in 0.7.0), so we can just have a single backing function. This does become slightly more annoying given the sapling_received_notes vs sent_notes split, but we can put off dealing with that to the Orchard work.
There was a problem hiding this comment.
I contemplated doing this as a single query using MINUS and/or UNION but I figure that if fetching the memos is done in a hot loop over all the notes, there's something wrong elsewhere.
|
Needs a rebase to fix merge conflicts after #889 was merged. |
In general, it is preferable to use globally relevant identifiers where possible. This PR removes the `WalletRead::TxRef` associated type in favor of using `TxId` directly for the transaction identifier, and restricts the use of the `NoteRef` type to those scenarios where the result of one query is intended to be used directly as the input to another query. Closes zcash#834
Co-authored-by: str4d <[email protected]>
nuttycom
force-pushed
the
feature/tx_output_note_refs
branch
from
47ac5cb
to
f602ec1
Compare
This builds atop #886