refactor: add inclusion proofs to SyncTransactions output notes#1893
refactor: add inclusion proofs to SyncTransactions output notes#1893bobbinth merged 2 commits intoigamigo-include-chain-tipfrom
SyncTransactions output notes#1893Conversation
igamigo
changed the title
SyncTransactions output notesSyncTransactions output notes
igamigo
force-pushed
the
igamigo-tx-notes-proofs
branch
from
ac7a8ef to
38e52bb
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! I think this works for now. It does introduce some inconstancies, but I think we'll need to address them in v0.15. Let's create a follow-up issue for these.
| // Output notes of the transaction together with their inclusion proofs. | ||
| repeated note.NoteSyncRecord output_notes = 6; |
There was a problem hiding this comment.
I think this is probably fine for now, but this again "de-syncs" the TransactionHeader message from the TransactionHeader struct in the protocol.
Ideally, we'd fine a way to keep things consistent - for example, if we can't align this with TransactionHeader struct, we could rename this into TransactionRecord or something like this. There could also be other ways to group things. Let's create an issue for this to resolve this in v0.15.
| // The unique identifier of the transaction. | ||
| TransactionId transaction_id = 1; |
There was a problem hiding this comment.
Not related to this PR, but I thought we got rid of this filed (because it should be computable from other fields in this message) - but maybe we didn't quite get there?
There was a problem hiding this comment.
Yes, good point, I hadn't noticed this was still around. It should be computable, just double checked. I can remove it here or add it to the issue for refactoring the transaction record.
There was a problem hiding this comment.
Let's keep it as is for now and address it in a later PR.
There was a problem hiding this comment.
What I mentioned above is not quite true, as NoteSyncRecord does not contain full note metadata. Expanded here: #1898 (comment)
| proto::rpc::TransactionRecord { | ||
| header: Some(proto::transaction::TransactionHeader { | ||
| transaction_id: Some(self.transaction_id.into()), | ||
| account_id: Some(self.account_id.into()), | ||
| initial_state_commitment: Some(self.initial_state_commitment.into()), | ||
| final_state_commitment: Some(self.final_state_commitment.into()), | ||
| input_notes: self.input_notes.into_iter().map(Into::into).collect(), | ||
| output_notes: self.output_notes.into_iter().map(Into::into).collect(), | ||
| fee: Some(Asset::from(self.fee).into()), | ||
| }), | ||
| block_num: self.block_num.as_u32(), | ||
| } |
There was a problem hiding this comment.
Related to one of the previous comments, I wonder if the note inclusion paths could be a part of the TransactionRecord rather than TransactionHeader. But again, this is probably something to solve in a follow-up PR.
igamigo
force-pushed
the
igamigo-include-chain-tip
branch
2 times, most recently
from
9289dd9 to
7a07a95
Compare
igamigo
force-pushed
the
igamigo-tx-notes-proofs
branch
from
5d1cf37 to
60c063a
Compare
2 participants
Closes #1890 and builds on #1881
Enriches the response of
SyncTransactionsto include output note inclusion proofs. I did not find a simpler way to do this; alternatives included adding the data to the transactions tables or joining but this was not achievable with extra refactors or important downsides.Also includes minor related refactors.