Skip to content

fix(cli, store): pass account_id filter to list_consumable_notes and simplify sqlite get_setting - #2449

Open
yappermoar-boop wants to merge 1 commit into
0xMiden:nextfrom
yappermoar-boop:fix-cli-notes-account-id-filter-and-sqlite-settings
Open

fix(cli, store): pass account_id filter to list_consumable_notes and simplify sqlite get_setting#2449
yappermoar-boop wants to merge 1 commit into
0xMiden:nextfrom
yappermoar-boop:fix-cli-notes-account-id-filter-and-sqlite-settings

Conversation

@yappermoar-boop

Copy link
Copy Markdown

In bin/miden-cli/src/commands/notes.rs, passed account_id.as_ref() to list_consumable_notes when listing consumable notes so the --account-id CLI filter is respected. In crates/sqlite-store/src/settings.rs, simplified get_setting to take &Connection and query directly without opening an unnecessary transaction for a SELECT query.

@juan518munoz juan518munoz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All changes look ok, could you fix the conflicts and the CI please?

Comment on lines +80 to +81
NotesCmd { list: Some(NoteFilter::Consumable), account_id, .. } => {
list_consumable_notes(client, account_id.as_ref()).await?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a genuine fix

Comment on lines 15 to 24
pub(crate) fn get_setting<T: FromSql>(
conn: &mut Connection,
conn: &Connection,
name: &str,
) -> Result<Option<T>, StoreError> {
conn.transaction()
.into_store_error()?
.query_row("SELECT value FROM settings WHERE name = $1", params![name], |row| {
row.get(0)
})
.optional()
.into_store_error()
conn.query_row("SELECT value FROM settings WHERE name = $1", params![name], |row| {
row.get(0)
})
.optional()
.into_store_error()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Removing the mut Connection and using a transaction aligns with set_setting and remove_setting, so it's also ok.

Comment thread bin/miden-cli/src/commands/address.rs Outdated
@yappermoar-boop
yappermoar-boop force-pushed the fix-cli-notes-account-id-filter-and-sqlite-settings branch from 1eaede7 to 4120b51 Compare September 8, 2026 21:58
@yappermoar-boop

Copy link
Copy Markdown
Author

Rebased onto next to resolve the conflict with the new settings scope, and applied the suggested invalid tag_len wording. CI should run cleanly now.

@juan518munoz

Copy link
Copy Markdown
Collaborator

I have approved the CI workflows. Please add a changelog entry noting the fix.

@juan518munoz

Copy link
Copy Markdown
Collaborator

Also note that commits need to be signed to be able mergeable.

@yappermoar-boop
yappermoar-boop force-pushed the fix-cli-notes-account-id-filter-and-sqlite-settings branch from 4120b51 to 2c4ee53 Compare September 9, 2026 20:45
@yappermoar-boop
yappermoar-boop force-pushed the fix-cli-notes-account-id-filter-and-sqlite-settings branch from 2c4ee53 to 3ab9313 Compare September 9, 2026 20:47
@yappermoar-boop

Copy link
Copy Markdown
Author

Added the changelog entries under ## Unreleased -> ### Fixes, rebased onto the latest next, and signed the commit.

@juan518munoz

Copy link
Copy Markdown
Collaborator

Thanks! I have enabled the CI, please monitor that all checks succeed.

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