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

Improvements to *NetworkApiClient ergonomics #1578

Open
carver opened this issue Oct 31, 2024 · 1 comment
Open

Improvements to *NetworkApiClient ergonomics #1578

carver opened this issue Oct 31, 2024 · 1 comment
Labels
flamingo Maintenance or downtime for the person on Flamingo rotation to tackle

Comments

@carver
Copy link
Collaborator

carver commented Oct 31, 2024

Inspired by a bug I introduced in glados, and thoughts about how to make it less likely in the future:
ethereum/glados#335 (review)

The current usage pattern goes something like:

let key = StateContentKey::AccountTrieNode(...);
let response = match StateNetworkApiClient::get_content(transport, key) {
  Ok(response) => response,
  Err(e) => return handle_err(e),
};
let decoded = match StateContentValue::decode(key, response.content) {
  Ok(decoded) => decoded,
  Err(e) => return handle_err(e),
};
if let StateContentValue::TrieNode(trie_node) = decoded {
  // do_something(trie_node)
}

An ideal API probably looks something closer to this (at least as an option):

let key = StateContentKey::AccountTrieNode(...);
let trie_node = match StateNetworkApiClient::get_decoded_content(transport, key) {
  Ok(trie_node) => trie_node, // only a single type can be returned by any key type, so we can skip the enum
  Err(e) => return handle_err(e),
};
// do_something(trie_node)

I don't know the rust magic to make that happen ^ but it should be straightforward to do a middle-ground solution, like:

let key = StateContentKey::AccountTrieNode(...);
let trie_node = match StateNetworkApiClient::get_decoded_content(transport, key) {
  Ok(StateContentValue::TrieNode(trie_node)) => trie_node, // only a single type can be returned by any key type, so we can skip the enum
  Ok(wrong_type) => panic!("Should never get a different value when looking up a trie node. Got: {wrong_type}"),
  Err(e) => return handle_err(e),
};
// do_something(trie_node)

I do see that there could be reasons to keep both get_content and get_decoded_content around in the API. Maybe this is pretty low priority, since it amounts to having to make one less function call. (unless we can get the magic solution working)


At the very least, we can add some docs to EncodedTrieNode that say: if you're trying to decode this by hand after making an RPC request, you're probably doing it wrong, and should be using StateContentValue::decode() instead

@carver carver added the flamingo Maintenance or downtime for the person on Flamingo rotation to tackle label Oct 31, 2024
@morph-dev
Copy link
Collaborator

Somewhat related to this, we have logic in Census where we do something like this (also for other api calls, like ping, recursive_find_nodes):

match self.subnetwork {
    Subnetwork::History => {
        HistoryNetworkApiClient::find_nodes(&self.client, enr.clone(), distances)
    }
    Subnetwork::State => {
        StateNetworkApiClient::find_nodes(&self.client, enr.clone(), distances)
    }
    Subnetwork::Beacon => {
        BeaconNetworkApiClient::find_nodes(&self.client, enr.clone(), distances)
    }
    _ => unreachable!("find_nodes: unsupported subnetwork: {}", self.subnetwork),
}

The problem is that all of these functions (and many more) have the exact same signature but they belong to different traits.

I think it's possible to use some rust magic and create one trait that can be use to abstract this. In that case, it might be possible to add extra functionality, like the one that you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flamingo Maintenance or downtime for the person on Flamingo rotation to tackle
Projects
None yet
Development

No branches or pull requests

2 participants