Skip to content
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

Expose get_notes_by_id() to selectively get a note from a miden node #354

Closed
daedlock opened this issue May 19, 2024 · 10 comments
Closed

Expose get_notes_by_id() to selectively get a note from a miden node #354

daedlock opened this issue May 19, 2024 · 10 comments
Labels
enhancement New feature or request
Milestone

Comments

@daedlock
Copy link

daedlock commented May 19, 2024

Feature description

Right now, the only way to fetch notes from a node is via tags. However, this might not be the desired way of selectively fetching a specific note if other notes share the same tag. Exposing get_notes_by_id() to the public api could be beneficial.

Why is this feature needed?

If a client is interested in individual notes, they should not be forced to fetch and keep track of other notes sharing the same tag as this will add unneeded storage overhead.

@daedlock daedlock added the enhancement New feature or request label May 19, 2024
@igamigo igamigo added this to the v04 milestone May 23, 2024
@mFragaBA
Copy link
Contributor

Feature description

Right now, the only way to fetch notes from a node is via tags. However, this might not be the desired way of selectively fetching a specific note if other notes share the same tag. Exposing get_notes_by_id() to the public api could be beneficial.

Why is this feature needed?

If a client is interested in individual notes, they should not be forced to fetch and keep track of other notes sharing the same tag as this will add unneeded storage overhead.

@daedlock
Couple of questions.

  • What would the use case of get_notes_by_id be? Do you want to use it as a means of updating tracked notes states, or just to check if a note was committed/consumed but without tracking it with the client
  • Would the NoteDetails struct info be enough or is more info needed for that use case?
  • when you mention the overhead on:

    If a client is interested in individual notes, they should not be forced to fetch and keep track of other notes sharing the same tag as this will add unneeded storage overhead.

do you mean the network + memory overhead, or do you mean literally storing them in the database (this last one shouldn't be the case but perhaps we're missing a use case where this happens)?

Lastly if it's something that's blocking you there is a workaround to work with in the meantime which is to create a separate TonicRpcClient and do the requests manually. Not Ideal but you won't have to wait for the next release.

@bobbinth
Copy link
Contributor

Lastly if it's something that's blocking you there is a workaround to work with in the meantime which is to create a separate TonicRpcClient and do the requests manually. Not Ideal but you won't have to wait for the next release.

I had a similar question: would it make sense to use the TonicRpcClinet directly in this case instead of instantiating a full client?

@daedlock
Copy link
Author

daedlock commented May 31, 2024

A little bit of context: We are building an orderbook where there will be a backend service responsible for routing, matching orders in addition to maintaining and updating orderbook state. Traders will interface with such service to request information that will allow them to execute trades. A trader would typically receive note_ids they need to consume in order to fill their order.

What would the use case of get_notes_by_id be? Do you want to use it as a means of updating tracked notes states, or just to check if a note was committed/consumed but without tracking it with the client

Trader's client will use get_notes_by_id() against note_ids received from the backend service.

Would the NoteDetails struct info be enough or is more info needed for that use case?

Yes, I think so. The assets and recipient should be enough to allow traders to build consumption transactions.

do you mean the network + memory overhead, or do you mean literally storing them in the database (this last one shouldn't be the case but perhaps we're missing a use case where this happens)?

All of the above. Having the trader's client to track a static tag means that the client has to fetch, process, store all the notes which involves network, cpu, memory and storage. If I am not mistaken, this is an unneeded overhead specially when the orderbook gets bigger and bigger in the future.

Lastly if it's something that's blocking you there is a workaround to work with in the
meantime which is to create a separate TonicRpcClient and do the requests manually. Not Ideal but you won't have to wait for the next release.

Thanks! @bobbinth mentioned this to me yesterday. I will give it a try, but I assume I will have to take care of the deserialization part?

@igamigo
Copy link
Collaborator

igamigo commented May 31, 2024

All of the above. Having the trader's client to track a static tag means that the client has to fetch, process, store all the notes which involves network, cpu, memory and storage. If I am not mistaken, this is an unneeded overhead specially when the orderbook gets bigger and bigger in the future.

Yeah, the client does check that well-known note types (like P2ID) are consumable by the client's accounts before adding them to the store. However, right now, for more custom-type notes, it can't decide whether a note relevant so it stores them by default anyway.

Thanks! @bobbinth mentioned this to me yesterday. I will give it a try, but I assume I will have to take care of the deserialization part?

The TonicRpcClient already will handle deserialization for you (as will any client that implements the NodeRpcClient trait), but depending on your use-case you might have to adapt the response a bit.

@daedlock
Copy link
Author

daedlock commented May 31, 2024

, the client does check that well-known note types (like P2ID) are consumable by the client's accounts before adding them to the store

The notes in my use case are consumable by any account, it's a modified swap note basically representing a limit order.

The TonicRpcClient already will handle deserialization for you (as will any client that implements the NodeRpcClient trait), but depending on your use-case you might have to adapt the response a bit.

Got it thanks.

One question however, what are the cons/downsides of exposing the get_notes_by_id() to the lib user anyways? I am wondering since I know that the functionality already exists in the client.

@mFragaBA
Copy link
Contributor

One question however, what are the cons/downsides of exposing the get_notes_by_id() to the lib user anyways? I am wondering since I know that the functionality already exists in the client.

For me the only issue regarding that, is that we end up with different ways of achieving the same action (either instantiating the RPC client or calling get_notes_by_id through the client) instead of a standardized way. In my experience that usually leads to problems but I wouldn't say no just because of that. We can also stop exposing the TonicRpcClient and just use get_notes_by_id.
On the other side it's nice to have get_notes_by_id because in order to create another TonicRpcClient we'd need to pass around the config which is probably annoying anyways.

The TonicRpcClient mention was to unblock you in the meantime anyways.

Traders will interface with such service to request information that will allow them to execute trades. A trader would typically receive note_ids they need to consume in order to fill their order
[...]
The assets and recipient should be enough to allow traders to build consumption transactions.

Does the trader's client actually run a TX via the client? Or does it submit a TX request to the backend service and that takes care of executing the transaction? In case of the former, the store still needs to be updated with both the note's inclusion proof in its block and the block's inclusion proof in the Node's MMR in order to consume the note.

@bobbinth
Copy link
Contributor

I think this boils down to what is the reason for getting the note by ID:

  • If it is just to get some info about the note (without changing the state of the client) then using TonicRpcClient ma be the way to go.
  • If this is to prepare the note for future consumption by the client, then I think we should "import" the note. For this, we could use Client::import_input_note though it would require some modifications.
    • We are currently refactoring this to use a new NoteFile struct (see feat: Add NoteFile object miden-base#721) and maybe we should add NoteId variant there.
    • We should also probably rename this method into just Client::import_note.

@Dominik1999
Copy link
Collaborator

@daedlock did the above solve your issue?

@daedlock
Copy link
Author

daedlock commented Jun 17, 2024 via email

@Dominik1999
Copy link
Collaborator

Dominik1999 commented Jun 17, 2024

Is already being addressed as part of #375

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

5 participants