Add specific functions to RPC trait

[igamigo]

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]

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 #571

[SantiagoPittella]

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]

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]

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]

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):

• In fetch_public_note_details we do a get_note_details and build input note records from the received public notes. We could have something like get_public_note_records(&[note_id]) that returns a list of Unverified fully built InputNoteRecord (only possible if they are public).
• In get_updated_onchain_accounts we check for mismatched tracked public accounts and do a get_account_update for each. We could have a get_public_accounts(&[account_id]) that returns the list of public Accounts all at once that we can use to update the store.
• This is not a specific function but we should have a conversion between NoteInclusionProof and NoteInclusionDetails (maybe even remove the latter). We have a few places where we manually build the inclusion proof from the details.
• In get_foreign_account_inputs we call get_account_proofs with the include headers flag and then expect that the header fields are present. This check can be moved to a separate funtion that returns all of the details without being optional.

[tomyrd]

addressed by #616