-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add note type to NoteSyncRecord
#311
Add note type to NoteSyncRecord
#311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a couple of comments inline (only one of the is actionable for now).
3511d0f
to
8920ece
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you!
I did just realize that we need to make one more change: in select_notes_since_block_by_tag_and_sender
, we should remove the shift operations in SQL statement. We are now basically filtering by the entire tag. So, the SQL should look something like this:
SELECT
block_num,
batch_index,
note_index,
note_hash,
sender,
tag,
merkle_path,
details
FROM
notes
WHERE
-- find the next block which contains at least one note with a matching tag
block_num = (
SELECT
block_num
FROM
notes
WHERE
tag IN rarray(?1) OR sender IN rarray(?2)) AND
block_num > ?3
ORDER BY
block_num ASC
LIMIT
1
) AND
-- filter the block's notes and return only the ones matching the requested tags
tag IN rarray(?1) OR sender IN rarray(?2));
store/src/db/tests.rs
Outdated
@@ -624,7 +623,7 @@ fn test_notes() { | |||
// test no updates | |||
let res = sql::select_notes_since_block_by_tag_and_sender( | |||
&mut conn, | |||
&[(tag >> 48) as u32], | |||
&[(tag >> 16)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these shifts any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep thinking we are persisting u16
s. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
14a6b3e
into
phklive-block-producer-onchain-notes
Basing it off of
phklive-block-producer-onchain
to make it simpler to test from the client.