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

feat(katana): forked provider fetch remote non-state data #1700

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

Conversation

fishseabowl
Copy link
Contributor

@fishseabowl fishseabowl commented Mar 25, 2024

This PR adds the following functions to fetch non-state data from the fork network.

pub fn do_get_events(
        &self,
        filter: EventFilter,
        continuation_token: Option<String>,
        chunks_size: ChunkSize,
    ) -> Result<EventsPage, ForkedBackendError>

pub fn do_get_block_with_tx_hashes(
        &self,
        block_id: BlockIdOrTag,
    ) -> Result<starknet::core::types::MaybePendingBlockWithTxHashes, ForkedBackendError>

pub fn do_get_block_with_txs(
        &self,
        block_id: BlockIdOrTag,
    ) -> Result<starknet::core::types::MaybePendingBlockWithTxs, ForkedBackendError>

pub fn do_get_transaction_by_block_id_and_index(
        &self,
        block_id: BlockIdOrTag,
        index: TxNumber,
    ) -> Result<Transaction, ForkedBackendError>

pub fn do_get_transaction_by_hash(
        &self,
        transaction_hash: TxHash
    ) -> Result<Transaction, ForkedBackendError>

pub fn do_get_transaction_receipt(
        &self,
        transaction_hash: TxHash
    ) -> Result<starknet::core::types::MaybePendingTransactionReceipt, ForkedBackendError>

Closes #1466
Closes DOJ-128

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 147 lines in your changes missing coverage. Please review.

Project coverage is 67.93%. Comparing base (328004d) to head (2ff6312).
Report is 38 commits behind head on main.

Current head 2ff6312 differs from pull request most recent head 0423f89

Please upload reports for the commit 0423f89 to get more accurate results.

Files Patch % Lines
...ana/storage/provider/src/providers/fork/backend.rs 0.00% 147 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1700      +/-   ##
==========================================
- Coverage   68.83%   67.93%   -0.91%     
==========================================
  Files         318      274      -44     
  Lines       38068    29863    -8205     
==========================================
- Hits        26205    20286    -5919     
+ Misses      11863     9577    -2286     

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

@glihm glihm added katana This issue is related to Katana external-contributor labels Mar 26, 2024
@fishseabowl fishseabowl marked this pull request as draft March 26, 2024 06:22
Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

@fishseabowl thanks for the work done here!
On my end I was to suggest using a constant for the logger Target. Beyond than that, @kariy could you have a look to it? :)

@kariy kariy self-requested a review April 3, 2024 14:50
@fishseabowl fishseabowl marked this pull request as ready for review April 7, 2024 04:53
Comment on lines +890 to +891
from_block: Some(starknet::core::types::BlockId::Tag(BlockTag::Latest)),
to_block: Some(starknet::core::types::BlockId::Tag(BlockTag::Latest)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't pending that you want to use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glihm Thank you for your comments. Could you share more details or relevant documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently you're reading BlockTag::Latest, and I was wondering if instead we want to use BlockTag::Pending.
But re-reading your test, I think it's better what you've did with the latest. @kariy could you confirm?

@kariy kariy changed the title fetch non-state data feat(katana): forked provider fetch remote non-state data Apr 13, 2024
Copy link
Member

@kariy kariy left a comment

Choose a reason for hiding this comment

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

sorry for the late review @fishseabowl i forgot about this 🙏 .

i realized that this is a bit more complex than what i had anticipated. due to how katana's storage provider works, we cant exactly map the RPC calls to one of the forked backend requests. it needs to call the same provider traits methods just like the other provider types.

for example, when the getBlockWithTxs is called on the RPC server,

async fn block_with_txs(&self, block_id: BlockIdOrTag) -> RpcResult<MaybePendingBlockWithTxs> {

it needs to go through all these provider methods (ie BlockHashProvider::block_hash_by_id, BlockProvider::block, and BlockStatusProvider::block_status) to build the right return type that the RPC expect.

pub fn build(self) -> ProviderResult<Option<BlockWithTxs>> {
let Some(hash) = BlockHashProvider::block_hash_by_id(&self.provider, self.block_id)? else {
return Ok(None);
};
let block = BlockProvider::block(&self.provider, self.block_id)?
.expect("should exist if hash exists");
let finality_status = BlockStatusProvider::block_status(&self.provider, self.block_id)?
.expect("should exist if block exists");
Ok(Some(BlockWithTxs::new(hash, block, finality_status)))
}

this works well for non-forked provider types but for forked provider, ideally we want to just call the appropriate RPC methods to the forked network and just return the result without any modification as the type is already in the RPC format.

i still think this is possible, we just have to do some redundant storage operations in order to conform to the provider abstraction that we have. though, it may requires storing the fetched remote data, which involves refactoring the ForkedProvider storage layout.

does that make sense ?

@fishseabowl
Copy link
Contributor Author

sorry for the late review @fishseabowl i forgot about this 🙏 .

i realized that this is a bit more complex than what i had anticipated. due to how katana's storage provider works, we cant exactly map the RPC calls to one of the forked backend requests. it needs to call the same provider traits methods just like the other provider types.

for example, when the getBlockWithTxs is called on the RPC server,

async fn block_with_txs(&self, block_id: BlockIdOrTag) -> RpcResult<MaybePendingBlockWithTxs> {

it needs to go through all these provider methods (ie BlockHashProvider::block_hash_by_id, BlockProvider::block, and BlockStatusProvider::block_status) to build the right return type that the RPC expect.

pub fn build(self) -> ProviderResult<Option<BlockWithTxs>> {
let Some(hash) = BlockHashProvider::block_hash_by_id(&self.provider, self.block_id)? else {
return Ok(None);
};
let block = BlockProvider::block(&self.provider, self.block_id)?
.expect("should exist if hash exists");
let finality_status = BlockStatusProvider::block_status(&self.provider, self.block_id)?
.expect("should exist if block exists");
Ok(Some(BlockWithTxs::new(hash, block, finality_status)))
}

this works well for non-forked provider types but for forked provider, ideally we want to just call the appropriate RPC methods to the forked network and just return the result without any modification as the type is already in the RPC format.

i still think this is possible, we just have to do some redundant storage operations in order to conform to the provider abstraction that we have. though, it may requires storing the fetched remote data, which involves refactoring the ForkedProvider storage layout.

does that make sense ?

@kariy Thank you for your comments. Let me try this.

@glihm
Copy link
Collaborator

glihm commented Jun 20, 2024

@kariy how do you feel about this? Do you think we definitively need more rework to support that? Or @fishseabowl suggestion can be a good candidate?

@fishseabowl
Copy link
Contributor Author

@kariy how do you feel about this? Do you think we definitively need more rework to support that? Or @fishseabowl suggestion can be a good candidate?

@glihm @kariy I have updated the relevant code, can you help check what is missing? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor katana This issue is related to Katana
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add fork provider for non-state data
3 participants