Conversation
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
2 times, most recently
from
dbef86c to
3e51cba
Compare
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
7 times, most recently
from
0c13ead to
7115c71
Compare
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
from
7115c71 to
9b43e61
Compare
lianad/src/database/sqlite/mod.rs
Outdated
| .expect("Database must be available") | ||
| } | ||
|
|
||
| pub fn insert_external_coins<'a>( |
There was a problem hiding this comment.
Lets keep the function name payjoin specific.
Something like: inserts_outpoint_seen_before
lianad/src/database/sqlite/schema.rs
Outdated
| txid BLOB NOT NULL, | ||
| vout INTEGER NOT NULL, |
There was a problem hiding this comment.
Can we combine this to just be outpoint (txid:vout)
There was a problem hiding this comment.
outpoint BLOB NOT NULL PRIMARY KEYThere was a problem hiding this comment.
And I think rust-bitcoin outpoint implements consensus encode / decode
lianad/src/database/mod.rs
Outdated
| .collect() | ||
| } | ||
|
|
||
| fn insert_input_seen_before(&mut self, outpoints: &[bitcoin::OutPoint]) -> Option<bool> { |
There was a problem hiding this comment.
I think the return type should just be bool?
lianad/src/database/sqlite/mod.rs
Outdated
| &mut self, | ||
| outpoints: &[bitcoin::OutPoint], | ||
| ) -> Vec<DbPayjoinOutpoint> { | ||
| // SELECT * FROM payjoinoutpoints WHERE (txid, vout) IN ((txidA, voutA), (txidB, voutB)); |
There was a problem hiding this comment.
If we just have one col for outpoint I dont think we need to do an IN condition. We can just do equality on the PK which is more performant.
lianad/src/database/sqlite/schema.rs
Outdated
| CREATE TABLE payjoinoutpoints ( | ||
| txid BLOB NOT NULL, | ||
| vout INTEGER NOT NULL, | ||
| added_at INTEGER |
There was a problem hiding this comment.
Should this also be not null?
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
4 times, most recently
from
5400bed to
418a758
Compare
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
I just a few questions and a proposal to use insert or ignore.
lianad/src/database/mod.rs
Outdated
| ) -> HashMap<bitcoin::OutPoint, Option<u32>> { | ||
| self.get_seen_payjoin_outpoints(outpoints) | ||
| .into_iter() | ||
| .map(|db_pjoutpoint| (db_pjoutpoint.outpoint, db_pjoutpoint.added_at)) |
There was a problem hiding this comment.
Nit. This var is a bit hard to read. I would just go for ot.
| .map(|db_pjoutpoint| (db_pjoutpoint.outpoint, db_pjoutpoint.added_at)) | |
| .map(|ot| (ot.outpoint, ot.added_at)) |
lianad/src/database/sqlite/mod.rs
Outdated
| "INSERT INTO payjoinoutpoints (txid, vout, added_at) \ | ||
| VALUES (?1, ?2, ?3)", | ||
| rusqlite::params![outpoint.txid[..].to_vec(), outpoint.vout, curr_timestamp()], |
There was a problem hiding this comment.
Something is not adding up to me. The schema has a single row for outpoint but here we are adding the two components seperatetly? There is probably some magic transformation somewhere?
There was a problem hiding this comment.
Oops I was a little hasty and didn't transition everything over to our new schema format, I'm surprised it actually worked..
There was a problem hiding this comment.
might be bc the old db tables still exist with the previous schema?
lianad/src/database/sqlite/mod.rs
Outdated
| db_exec(&mut self.conn, |db_tx| { | ||
| for outpoint in outpoints { | ||
| db_tx.execute( | ||
| "INSERT INTO payjoinoutpoints (txid, vout, added_at) \ |
There was a problem hiding this comment.
Also should have added this to the prev review. Apologies I just thought of it .
Can we use Insert or ignore here. I think that works since outpoint is the primary key and pk has to be unique.
let rows_affected = conn.execute(
"INSERT OR IGNORE INTO ...",
params![...],
)?;Chat gpt says the execution returns the rows affected which would allow us to remove the get_seen_payjoin_outpoints entirely.
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
from
418a758 to
5eb98f0
Compare
liana did not have a way to add coins from external wallets to the db, this adds those coins to the db and forces is_from_self to be false and sets the wallet_id to 2 which is never the users wallet as it is hardcoded to 1.
benalleng
force-pushed
the
payjoin-v4-inputs-seen
branch
from
5eb98f0 to
27e0763
Compare
2 participants
This adds the remaining closure in the state check which requires a little more logic as we now need to reference a new db table to see what coins have been seen before.