Implement `get_notes_by_id` endpoint #298

phklive · 2024-04-04T11:20:16Z

In this PR I propose the addition of the get_notes_by_id endpoint on the RPC and Store of the Miden node

Closes: #286

Now making this request to the RPC:

{
 "note_ids": [
   {
     "d0": "405316859436263090",
     "d1": "10067701279746667862",
     "d2": "2555050322175287978",
     "d3": "14994459379969491785"
   },
   {
     "d0": "14343633916517334960",
     "d1": "4263043073880214079",
     "d2": "426630156430625624",
     "d3": "13403889746530043130"
   }
 ]
}

Returns this response:

{
  "notes": [
    {
      "block_num": 16,
      "note_index": 0,
      "note_id": {
        "d0": "405316859436263090",
        "d1": "10067701279746667862",
        "d2": "2555050322175287978",
        "d3": "14994459379969491785"
      },
      "sender": {
        "id": "11177765675888626144"
      },
      "tag": "6895328587244957060",
      "merkle_path": {
        "siblings": [
          {
            "d0": "6895328587244957060",
            "d1": "11177765675888626144",
            "d2": "0",
            "d3": "0"
          },
          // ... (omitted for brevity)
        ]
      }
    },
    {
      "block_num": 1186,
      "note_index": 0,
      "note_id": {
        "d0": "14343633916517334960",
        "d1": "4263043073880214079",
        "d2": "426630156430625624",
        "d3": "13403889746530043130"
      },
      "sender": {
        "id": "10737277274808842422"
      },
      "tag": "5202715297868241916",
      "merkle_path": {
        "siblings": [
          {
            "d0": "5202715297868241916",
            "d1": "10737277274808842422",
            "d2": "0",
            "d3": "0"
          },
          // ... (omitted for brevity)
        ]
      }
    }
  ]
}

Proving the functionality of the endpoint.

igamigo

LGTM. I'm guessing the changes to the storage (for supporting on-chain note data) will also imply a couple of changes in the endpoint.

store/README.md

igamigo · 2024-04-04T15:28:30Z

store/src/db/sql.rs

+/// Select Note's matching the NoteId using the given [Connection].
+///
+/// # Returns
+///
+/// - Empty vector if no matching `note`.
+/// - Otherwise, notes which `note_hash` matches the `NoteId` as bytes.


Should this include a description of how off-chain and on-chain notes are different in terms of the response? (or perhaps when the storage changes are made)

igamigo · 2024-04-04T15:29:20Z

store/src/db/sql.rs

+
+    let placeholders = note_ids.iter().map(|_| "?").collect::<Vec<&str>>().join(", ");
+
+    let query = &format!("SELECT * FROM notes WHERE note_hash IN ({})", placeholders);


The note_hash column name should likely be updated to note_id but not for this set of changes

Instead of formatting the SQL statement, this should use the rarray extension. Here is an example:

miden-node/store/src/db/sql.rs

Lines 83 to 96 in 0335339

let mut stmt = conn.prepare(

"

SELECT

account_id, account_hash, block_num

FROM

accounts

WHERE

block_num > ?1 AND

block_num <= ?2 AND

account_id IN rarray(?3)

ORDER BY

block_num ASC

",

)?;

In general I'm against formatting SQL statements, for two reasons:

It is an easy way to get into a SQL injection vulnerability

It makes it harder to have caches of prepared statements

Fixed it using the rarray method.

The note_hash column name should likely be updated to note_id but not for this set of changes

Opening an issue here: #301

hackaugusto · 2024-04-04T15:31:29Z

proto/proto/responses.proto

@@ -99,6 +99,11 @@ message GetTransactionInputsResponse {

 message SubmitProvenTransactionResponse {}

+message GetNotesByIdResponse {
+    // Lists Note's returned by the database
+    repeated note.Note notes = 1;


It seems these are missing the details (i.e. the note's script/inputs/etc.):

miden-node/proto/proto/note.proto

Lines 8 to 15 in 0335339

message Note {

fixed32 block_num = 1;

uint32 note_index = 2;

digest.Digest note_id = 3;

account.AccountId sender = 4;

fixed64 tag = 5;

merkle.MerklePath merkle_path = 7;

}

I don't think that this message is missing a details field.

This message represents getting a list of notes. Each note will have their own details field indeed. But the GetNoteById message should not return details but only a list of notes.

If you want to see the Note message with a details field, then see this PR when I make the modification to the db: #300

hackaugusto · 2024-04-04T15:36:32Z

store/src/db/sql.rs

+
+    let placeholders = note_ids.iter().map(|_| "?").collect::<Vec<&str>>().join(", ");
+
+    let query = &format!("SELECT * FROM notes WHERE note_hash IN ({})", placeholders);


Instead of formatting the SQL statement, this should use the rarray extension. Here is an example:

miden-node/store/src/db/sql.rs

Lines 83 to 96 in 0335339

let mut stmt = conn.prepare(

"

SELECT

account_id, account_hash, block_num

FROM

accounts

WHERE

block_num > ?1 AND

block_num <= ?2 AND

account_id IN rarray(?3)

ORDER BY

block_num ASC

",

)?;

In general I'm against formatting SQL statements, for two reasons:

It is an easy way to get into a SQL injection vulnerability

It makes it harder to have caches of prepared statements

proto/proto/rpc.proto

Dominik1999 · 2024-04-05T16:13:25Z

merge?

phklive · 2024-04-05T16:20:15Z

merge?

Hey, yes soon. Still working on making sure that everything works correctly and addressing the small nits from the other teammates.

bobbinth

Looks good! Thank you! I left a couple of doc-related comments inline. Other than that, we should also resolve the merge conflicts (hopefully, shouldn't be too difficult) and then we can merge.

store/README.md

rpc/README.md

bobbinth

All looks good! Thank you!

* Getting notes by id works * lint and test * Improved comments + Added conversion from digest::Digest to NoteId * Added rpc validation for NoteId's * Added documentation to Readmes * Lint * Fixed ci problems * Added test for endpoint + refactored sql query + improved documentation * Fixed lint * Order rpc and store endpoints alphabitically * Improved documentation with gh comments

phklive added 6 commits April 4, 2024 11:11

Getting notes by id works

7c09397

lint and test

24c05ab

Improved comments + Added conversion from digest::Digest to NoteId

c52dafe

Added rpc validation for NoteId's

626cb54

Added documentation to Readmes

0d80b8b

Lint

0335339

phklive marked this pull request as ready for review April 4, 2024 15:02

phklive requested review from hackaugusto, polydez and bobbinth and removed request for hackaugusto April 4, 2024 15:02

igamigo linked an issue Apr 4, 2024 that may be closed by this pull request

Implement GetNotesById endpoint #286

Closed

igamigo approved these changes Apr 4, 2024

View reviewed changes

hackaugusto reviewed Apr 4, 2024

View reviewed changes

bobbinth reviewed Apr 5, 2024

View reviewed changes

proto/proto/rpc.proto Outdated Show resolved Hide resolved

phklive added 2 commits April 5, 2024 14:59

Merge next + lint

1a4a829

Fixed ci problems

e0572ba

Added test for endpoint + refactored sql query + improved documentation

a6bd2ea

phklive added 3 commits April 5, 2024 20:12

Fixed lint

a32b312

Order rpc and store endpoints alphabitically

f9b4a0b

Merge branch 'next' into phklive-get-notes-by-id

3c5a9fd

bobbinth approved these changes Apr 7, 2024

View reviewed changes

store/README.md Outdated Show resolved Hide resolved

rpc/README.md Outdated Show resolved Hide resolved

phklive added 2 commits April 8, 2024 20:14

Rebased from next

91c7384

Improved documentation with gh comments

9a08d1e

bobbinth approved these changes Apr 8, 2024

View reviewed changes

phklive merged commit f9264ac into next Apr 8, 2024
5 checks passed

phklive deleted the phklive-get-notes-by-id branch April 8, 2024 19:51

Dominik1999 mentioned this pull request Apr 9, 2024

Implement GetNotesById endpoint #286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `get_notes_by_id` endpoint #298

Implement `get_notes_by_id` endpoint #298

phklive commented Apr 4, 2024 •

edited

Loading

igamigo left a comment •

edited

Loading

igamigo Apr 4, 2024

igamigo Apr 4, 2024

hackaugusto Apr 4, 2024

phklive Apr 5, 2024

phklive Apr 5, 2024

hackaugusto Apr 4, 2024

phklive Apr 5, 2024

hackaugusto Apr 4, 2024

Dominik1999 commented Apr 5, 2024

phklive commented Apr 5, 2024

bobbinth left a comment

bobbinth left a comment


		let placeholders = note_ids.iter().map(\|_\| "?").collect::<Vec<&str>>().join(", ");

		let query = &format!("SELECT * FROM notes WHERE note_hash IN ({})", placeholders);

	let mut stmt = conn.prepare(
	"
	SELECT
	account_id, account_hash, block_num
	FROM
	accounts
	WHERE
	block_num > ?1 AND
	block_num <= ?2 AND
	account_id IN rarray(?3)
	ORDER BY
	block_num ASC
	",
	)?;

	message Note {
	fixed32 block_num = 1;
	uint32 note_index = 2;
	digest.Digest note_id = 3;
	account.AccountId sender = 4;
	fixed64 tag = 5;
	merkle.MerklePath merkle_path = 7;
	}

Implement get_notes_by_id endpoint #298

Implement get_notes_by_id endpoint #298

Conversation

phklive commented Apr 4, 2024 • edited Loading

igamigo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dominik1999 commented Apr 5, 2024

phklive commented Apr 5, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Implement `get_notes_by_id` endpoint #298

Implement `get_notes_by_id` endpoint #298

phklive commented Apr 4, 2024 •

edited

Loading

igamigo left a comment •

edited

Loading