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

Add support to Pathfinder v0_8 #456

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

GMKrieger
Copy link
Collaborator

Issue Number: N/A

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Adds support for pathfinder v0_8. It coalesces the 2 types of request on the client and then converts the new version to the one supported by snos.

Breaking changes?

  • yes
  • no

@GMKrieger GMKrieger self-assigned this Dec 30, 2024
Copy link
Collaborator

@whichqua whichqua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts, let me know what you think.


#[derive(Debug)]
pub enum ClientVersion {
Rpcv07(String),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the String here? The enum should handle it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to #456 (comment)

fn new(base_url: &str) -> Self {
let starknet_rpc_url = format!("{}/rpc/v0_7", base_url);
fn new(base_url: &str, version: ClientVersion) -> Self {
let rpc_version = match &version {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, implement Display for ClientVersion

Rpcv08(String),
}

impl TryFrom<&str> for ClientVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing with impl FromStr because it allows more flexibility dealing with different string types.

#[case::missing_constant_max_high(164684)]
#[case::retdata_not_a_relocatable(160033)]
#[case::get_tx_info_using_ptr_over_relocatable(243766)]
#[case::small_block_with_only_invoke_txs(76793, "v0_7")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean these tests cannot be run on v0.8?


pub fn convert_storage_to_pathfinder_class_proof(storage_proof: StorageProof) -> PathfinderClassProof {
let mut class_proof: Vec<TrieNode> = Vec::new();
for node in &storage_proof.classes_proof {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a more declarative approach

Suggested change
for node in &storage_proof.classes_proof {
let class_proof = storage_proof
.classes_proof
.iter()
.map(convert_to_trie_node)
.collect();

}

pub fn convert_storage_to_pathfinder_proof(storage_proof: StorageProof) -> PathfinderProof {
let mut contract_proof: Vec<TrieNode> = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a more declarative approach

Suggested change
let mut contract_proof: Vec<TrieNode> = Vec::new();
let contract_proof: Vec<TrieNode> = storage_proof.contracts_proof.nodes
.iter()
.map(|node| convert_to_trie_node(node))
.collect();
let storage_proofs: Vec<Vec<TrieNode>> = storage_proof.contracts_storage_proofs
.iter()
.map(|nodes| {
nodes
.iter()
.map(|node| convert_to_trie_node(node))
.collect()
})
.collect();

storage_proofs.push(trie_nodes);
}

let contract_data: Option<ContractData>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should help you avoid the if:

let contract_data = storage_proof
    .contracts_storage_proofs
    // Get the first "proof" only if it exists
    .get(0)
    // Filter out if the first proof is empty
    .filter(|proof_nodes| !proof_nodes.is_empty())
    .map(|proof_nodes| {
        let first_node = &proof_nodes[0];
        let root = first_node.node_hash;

        match &first_node.node {
            MerkleNode::Binary { left, .. } if left.to_hex_string() == "0x0" => {
                // If left is "0x0", then storage_proofs is empty
                ContractData {
                    root,
                    storage_proofs: vec![],
                }
            }
            // For all other cases (including `MerkleNode::Edge`),
            // store the computed `storage_proofs`.
            _ => ContractData { root, storage_proofs },
        }
    });

match node.node {
MerkleNode::Binary { left, right } => TrieNode::Binary { left, right },
MerkleNode::Edge { child, path, length } => {
TrieNode::Edge { child, path: EdgePath { len: length.try_into().unwrap(), value: path } }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either propagate this error up, or use expect

Suggested change
TrieNode::Edge { child, path: EdgePath { len: length.try_into().unwrap(), value: path } }
TrieNode::Edge { child, path: EdgePath { len: length.try_into().expect("length must be a valid integer"), value: path } }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants