-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: smt code review #5876
feat: smt code review #5876
Conversation
Test Results (CI)1 233 tests - 2 1 233 ✔️ - 2 11m 31s ⏱️ -15s Results for commit f0b04cc. ± Comparison against base commit 7fc3a8f. This pull request removes 2 tests.
♻️ This comment has been updated with latest results. |
@@ -1828,10 +1828,10 @@ impl BlockchainBackend for LMDBDatabase { | |||
} | |||
} | |||
|
|||
fn fetch_utxos_in_block( | |||
fn fetch_outputs_in_block_with_spend_state( |
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.
Maybe add a function comment to say true
in the tuple means spent.
@@ -457,13 +451,13 @@ where B: BlockchainBackend | |||
db.fetch_kernels_in_block(&hash) | |||
} | |||
|
|||
pub fn fetch_utxos_in_block( | |||
pub fn fetch_outputs_in_block_with_spend_state( |
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.
Maybe add a function comment to say true
in the tuple means spent.
@@ -267,22 +267,22 @@ impl BlockchainBackend for TempDatabase { | |||
self.db.as_ref().unwrap().fetch_kernel_by_excess_sig(excess_sig) | |||
} | |||
|
|||
fn fetch_utxos_in_block( | |||
fn fetch_outputs_in_block_with_spend_state( |
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.
Maybe add a function comment to say true
in the tuple means spent.
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 feel this is a huge improvement in priming the reader for context. Just a couple of suggestions ^^.
Test Results (Integration tests) 2 files ±0 11 suites ±0 2h 13m 1s ⏱️ + 5m 16s For more details on these failures, see this check. Results for commit f0b04cc. ± Comparison against base commit 7fc3a8f. ♻️ This comment has been updated with latest results. |
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Description --- Updates the SMT branch with code review --------- Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Description --- Updates the SMT branch with code review --------- Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Description --- Updates the SMT branch with code review --------- Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Description
Updates the SMT branch with code review