Codecov Report

Attention: Patch coverage is 75.43860% with 70 lines in your changes missing coverage. Please review.

Project coverage is 74.53%. Comparing base (4ff174d) to head (d4b65b1).

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
+ Coverage   74.38%   74.53%   +0.15%     
==========================================
  Files         109      111       +2     
  Lines       11493    11747     +254     
==========================================
+ Hits         8549     8756     +207     
- Misses       2278     2318      +40     
- Partials      666      673       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Differences / Issues spotted comparing with PF

1. Params are not optional (why?)

Request

{
  "jsonrpc": "2.0",
  "method": "starknet_getStorageProof",
  "params": [
    "latest",
    ["0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e"]
  ],
  "id": 1
}

Response
"message": "Invalid Params",

2. Response does not filter out duplicate nodes in hash HashToNode mapping

Request

{
  "jsonrpc": "2.0",
  "method": "starknet_getStorageProof",
  "params": [
    "latest",
    ["0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e", 
     "0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e"],
    [],
    []
  ],
  "id": 1
}

Response

{
  "jsonrpc": "2.0",
  "result": {
    "classes_proof": [
      {
        "node_hash": "0x217d3fbc3bccaca79f86feb5e9dbbfa531ab481503594df9ca426245a65a12f",
        "node": { ... }
      },
// ...
      {
        "node_hash": "0x1c2825874f9b479403ea627428397852d86c67d955be072d68de143a98bb923",
        "node": { ...
          "child": "0x35f5f9b5523cdbd190f2d7a7310aadcfcf0231a9a580786b9cd5db8b512de2"
        }
      },
      {
        "node_hash": "0x217d3fbc3bccaca79f86feb5e9dbbfa531ab481503594df9ca426245a65a12f",
         "node": { ... }
      },
// ...
      {
        "node_hash": "0x1c2825874f9b479403ea627428397852d86c67d955be072d68de143a98bb923",
        "node": { ...
          "child": "0x35f5f9b5523cdbd190f2d7a7310aadcfcf0231a9a580786b9cd5db8b512de2"
        }
      }
    ],
  // ...
  "id": 1
}

I discussed this issue with @kirugan and we agreed that we address this once we allow juno to prove historical states

So there are 2 approaches we can take:

1. Implements TrieReader interface and return HeadTrie in Blockchain Reader interface
2. Add Trie access methods in StateReader interface

For the 1st approach, we can agree on the architecture such that a Blockchain Reader shouldn't know the underlying State implementation (i.e. Trie). Adding HeadTrie method to Reader indicates we are coupling Trie and Blockchain together, which shouldn't happen. So essentially this is a technical debt.

For the 2nd approach, the architecture makes sense, the issue is that we have an unsupported feature for historical trie access. It's unsure when we will support historical trie access as it's a huge refactor on the state, trie and db modules.

I'd rather go with an unsupported feature and detect it early rather than incur technical debts and make the modules even more coupled and harder to refactor.