Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ec2/txn table #5

Merged
merged 11 commits into from
Aug 27, 2024
Merged

Ec2/txn table #5

merged 11 commits into from
Aug 27, 2024

Conversation

ec2
Copy link
Member

@ec2 ec2 commented Aug 23, 2024

No description provided.

Copy link
Collaborator

@willemolding willemolding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few little comments but looking amazing I think we are really close

/// Keeps track of notes that are spent in which transaction
pub struct ReceievdNoteSpends(HashMap<NoteId, TxId>);

pub struct ReceivedNoteTable(pub Vec<ReceivedNote>);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could encapsulate this if we can

Suggested change
pub struct ReceivedNoteTable(pub Vec<ReceivedNote>);
pub struct ReceivedNoteTable(Vec<ReceivedNote>);

pub struct ReceivedNote {
// Uniquely identifies this note
note_id: NoteId,
txid: TxId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this if it contains duplicate data. Otherwise add a doc comment explaining why we need it

account_id: AccountId,
//sapling: (diversifier, value, rcm) orchard: (diversifier, value, rho, rseed)
note: Note,
nf: Option<Nullifier>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment describing the conditions under which nullifier is None and when it is Some

// created: String,
/// Combines block height and mined_height into a txn status
tx_status: TransactionStatus,
tx_index: Option<u32>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the index in the transactions table or how it is referenced in the blockchain? Very confused about these

note_id: NoteId,
output: &WalletSaplingOutput<AccountId>,
) {
// let note_id = NoteId::new(txid, ShieldedProtocol::Sapling, output.index() as u16);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// let note_id = NoteId::new(txid, ShieldedProtocol::Sapling, output.index() as u16);

note_id: NoteId,
output: &WalletOrchardOutput<AccountId>,
) {
// let note_id = NoteId::new(txid, ShieldedProtocol::Orchard, output.index() as u16);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// let note_id = NoteId::new(txid, ShieldedProtocol::Orchard, output.index() as u16);

Comment on lines 278 to 292
pub fn mark_sapling_note_spent(&mut self, nf: sapling::Nullifier, txid: TxId) {
// let note = ReceivedNote {
// txid,
// output_index: todo!(),
// account_id: todo!(),
// note: todo!(),
// nf: todo!(),
// is_change: todo!(),
// memo: todo!(),
// commitment_tree_position: todo!(),
// recipient_key_scope: todo!(),
// };
// self.sapling_spends.insert(nf, (txid, true));
todo!()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn mark_sapling_note_spent(&mut self, nf: sapling::Nullifier, txid: TxId) {
// let note = ReceivedNote {
// txid,
// output_index: todo!(),
// account_id: todo!(),
// note: todo!(),
// nf: todo!(),
// is_change: todo!(),
// memo: todo!(),
// commitment_tree_position: todo!(),
// recipient_key_scope: todo!(),
// };
// self.sapling_spends.insert(nf, (txid, true));
todo!()
}

// Mark transparent UTXOs as spent
#[cfg(feature = "transparent-inputs")]
for utxo_outpoint in sent_tx.utxos_spent() {
// self.mark_transparent_utxo_spent(wdb.conn.0, tx_ref, utxo_outpoint)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// self.mark_transparent_utxo_spent(wdb.conn.0, tx_ref, utxo_outpoint)?;

for output in sent_tx.outputs() {
// insert sent output

// TODO: Do we actually have to store these notes and outputs?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do need to be added here so the wallet summary will update and the wallet will appear responsive. Zcash block time is really slow so otherwise the balance won't update for minutes and trying to build new transactions in this time will respend the same outputs.

We need to ensure the scanning logic can handle seeing the same note multiple times (once from here then once when scanning)

@ec2 ec2 merged commit fd077c1 into ec2/memwallet Aug 27, 2024
18 of 20 checks passed
@ec2 ec2 deleted the ec2/txn-table branch September 3, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants