0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
39 stars 30 forks source link

Add specific functions to RPC trait #573

Open igamigo opened 3 weeks ago

igamigo commented 3 weeks ago

Currently, most (if not all) of the functions included in the RPC trait are correlated 1:1 with the node RPC endpoints. However, it might be better if the trait functions were specifically suited for use-cases instead. For example, instead of using RPC's NodeRpcClient::get_account_proofs() -> (u32, Vec<AccountProof>) for foreign accounts (where AccountProof may or may not include headers, depending on what was requested to the node), we could instead add a specific function that returns data useful for FPI (ie, something like NodeRpcClient::get_accounts_for_fpi() -> (u32, Vec<FpiAccountData>) where FpiAccountData always contains account code, storage header and account header.

igamigo commented 2 weeks ago

As part of this, we should also move all RPC-related validations to the RPC implementations instead of in the client logic (example).

Also, related to this is https://github.com/0xPolygonMiden/miden-client/issues/571

SantiagoPittella commented 3 days ago

I've been thinking on this and don't believe this is the best approach. In my opinion, the NodeRpcClient should have a 1:1 correlation with the node's methods, serving as a straightforward interface to them. For domain-specific purposes, clients can combine these methods as needed.

I propose keeping NodeRpcClient focused solely on exposing the node's methods, while adding domain-specific logic to the client itself in the form of methods. However, I do believe that validations should be implemented within the NodeRpcClient methods.

I'd love to hear your thoughts on this.

cc @igamigo @bobbinth

igamigo commented 3 days ago

I think that should be fine. Either keeping the trait as it is, and maybe adding a new mod/struct that handles the more specific methods within it, or separating them on Client.

The other alternative (maybe more in line with what we do with Store for example) would be to keep more client-specific methods on the trait, but auto-implemented, so that implementors only have to specify the RPC methods and users can use those methods if needed from the same RPC structs.

bobbinth commented 3 days ago

I think proving some more specialized methods that rely on the "core" methods is good idea. The way to think about these is that the provide a subset of the functionality exposed by the core methods (assuming this subset is actually useful). But I wouldn't go beyond that and move business logic into the NodeRpcClient (e.g., I wouldn't add a method that combines 2 or more RPC calls and merges their responses together).

Not sure how many such methods there are, but I think the one in the original post is a good example. We have a more general get_account_proofs() method which is 1:1 with the RPC, and we can provide an implementation of a more specialized method get_accounts_for_fpi() that internally used get_accounts_for_fpi() in a more narrow fashion (and also provides some boilerplate error handling).

tomyrd commented 10 hours ago

I've been reviewing the places where we use the rpc functions. I think there are some places where we do an rpc call and then do some post processing which we could move to a specific function which uses the base rpc functions. Here are some of these examples I found (the names could be changed, I haven't put much thought into them):