Codecov Report

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

Project coverage is 74.47%. Comparing base (eb1f2ca) to head (7e56c76).

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
+ Coverage   74.33%   74.47%   +0.14%     
==========================================
  Files         110      112       +2     
  Lines       11531    11785     +254     
==========================================
+ Hits         8571     8777     +206     
- Misses       2290     2330      +40     
- Partials      670      678       +8     

☔ 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.