Dvir/save tx status #888

DvirYo-starkware · 2023-07-23T12:35:25Z

Pull Request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

Yes
No

Other information

This change is

DvirYo-starkware

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @dan-starkware, @nagmo-starkware, and @yair-starkware)

a discussion (no related file):

I made minor changes in the gateway just make it compile.
Because this pull request is after the serialization change of EntryPointOffset in starknet-api there are two tests in the gateway that fails. The fix is in Nevo pull request. I can rebase this pull request on top of it or fix this in a different pull request.

crates/papyrus_storage/src/body/mod.rs line 334 at r4 (raw file):

    block_number: BlockNumber,
) -> StorageResult<()> {
    for index in 0..block_body.transactions.len() {

This code will panic if the transaction vector and the execution status vector are in different lengths. In the node flow, this will not happen because we pass only a valid block body. I can add a check in the beginning and return an error if the length is different, but I think this is unnecessary.

nagmo-starkware · 2023-07-24T07:04:52Z

Current dependencies on/for this PR:

main
- PR Dvir/save tx status #888 👈
  - PR add v0.4.0 not connected to endpoint #877
    - PR docs: readme update #916
    - PR fix v3 spec based on git tag #914
    - PR connect v0.4.0 to api endpoint #881
      - PR fix(JSON-RPC): make the json schema validation fail when result is null #958
        
        PR feat(JSON-RPC): add support for v0.4.0-rc3 #959

This comment was auto-generated by Graphite.

codecov · 2023-07-24T07:11:05Z

Codecov Report

Merging #888 (d582610) into main (9201f79) will increase coverage by 0.04%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
+ Coverage   73.72%   73.77%   +0.04%     
==========================================
  Files          53       53              
  Lines        4560     4568       +8     
  Branches     4560     4568       +8     
==========================================
+ Hits         3362     3370       +8     
  Misses        544      544              
  Partials      654      654

Impacted Files	Coverage Δ
crates/papyrus_gateway/src/v0_3_0/api/api_impl.rs	`88.23% <ø> (ø)`
crates/papyrus_storage/src/lib.rs	`43.67% <ø> (ø)`
.../starknet_reader_client/src/objects/transaction.rs	`59.31% <ø> (ø)`
crates/papyrus_storage/src/body/mod.rs	`77.21% <88.88%> (+0.29%)`	⬆️
crates/papyrus_gateway/src/transaction.rs	`84.61% <100.00%> (ø)`
crates/papyrus_storage/src/ommer/mod.rs	`41.08% <100.00%> (ø)`
crates/papyrus_storage/src/serializers.rs	`83.48% <100.00%> (+0.14%)`	⬆️
crates/starknet_reader_client/src/objects/block.rs	`86.40% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DvirYo-starkware

Reviewable status: 0 of 14 files reviewed, 4 unresolved discussions (waiting on @dan-starkware, @nagmo-starkware, and @yair-starkware)

crates/papyrus_storage/src/body/body_test.rs line 86 at r9 (raw file):

    for (block_number, tx_offset, original_index) in tx_cases {
        let expected_tx: Option<(

I think the explicit type is unnecessary.

crates/papyrus_storage/src/body/mod.rs line 35 at r9 (raw file):

//!
//! let stored_body_transactions = reader.begin_ro_txn()?.get_block_transactions(BlockNumber(0))?;
//! let expected_transactions_with_exec_statuses = Block::default().body.transactions.into_iter().zip(Block::default().body.transaction_execution_statuses.into_iter()).collect::<Vec<_>>();

Use the "block" variable instead of creating the "Block::default().body" again.

dan-starkware

Reviewed 4 of 9 files at r1, 2 of 3 files at r6, 2 of 4 files at r7, 4 of 5 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DvirYo-starkware, @nagmo-starkware, and @yair-starkware)

crates/papyrus_gateway/src/transaction.rs line 22 at r9 (raw file):

        .ok_or_else(|| ErrorObjectOwned::from(JsonRpcError::BlockNotFound))?;

    Ok(transactions.into_iter().map(|(tx, _)| Transaction::from(tx)).collect())

consider

Suggestion:

_execution_status

crates/papyrus_storage/src/body/body_test.rs line 89 at r9 (raw file):

            starknet_api::transaction::Transaction,
            starknet_api::transaction::TransactionExecutionStatus,
        )> = original_index.map(|i| (txs[i].clone(), tx_exec_sts[i].clone()));

How come now we need the type?

Code quote:

        let expected_tx: Option<(
            starknet_api::transaction::Transaction,
            starknet_api::transaction::TransactionExecutionStatus,
        )> = original_index.map(|i| (txs[i].clone(), tx_exec_sts[i].clone()));

crates/papyrus_storage/src/body/mod.rs line 334 at r4 (raw file):

Previously, DvirYo-starkware wrote…

This code will panic if the transaction vector and the execution status vector are in different lengths. In the node flow, this will not happen because we pass only a valid block body. I can add a check in the beginning and return an error if the length is different, but I think this is unnecessary.

Please add a 'panic' section

crates/papyrus_storage/src/body/mod.rs line 339 at r9 (raw file):

        let transaction_index = TransactionIndex(block_number, tx_offset_in_block);
        // A transaction must have an execution status.
        let tx = block_body.transactions[index].clone();

Can we avoid cloning here?

Code quote:

clone()

dan-starkware

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @DvirYo-starkware, @nagmo-starkware, and @yair-starkware)

DvirYo-starkware

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware, @nagmo-starkware, and @yair-starkware)

crates/papyrus_gateway/src/transaction.rs line 22 at r9 (raw file):

Previously, dan-starkware wrote…

consider

done

crates/papyrus_storage/src/body/body_test.rs line 86 at r9 (raw file):

Previously, DvirYo-starkware wrote…

I think the explicit type is unnecessary.

Done

crates/papyrus_storage/src/body/mod.rs line 334 at r4 (raw file):

Previously, dan-starkware wrote…

Please add a 'panic' section

Done.

crates/papyrus_storage/src/body/mod.rs line 339 at r9 (raw file):

Previously, dan-starkware wrote…

Can we avoid cloning here?

It will be done.

dan-starkware

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nagmo-starkware and @yair-starkware)

dan-starkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware and @yair-starkware)

DvirYo-starkware requested review from dan-starkware, yair-starkware and nagmo-starkware July 23, 2023 12:35

DvirYo-starkware self-assigned this Jul 23, 2023

DvirYo-starkware commented Jul 23, 2023

View reviewed changes

DvirYo-starkware and others added 5 commits July 24, 2023 09:52

Storage and test utils changes.

e867b70

Client changes.

f58cbe5

Gateway changes, just to make it compile.

4e5c060

Minor changes.

c9746ea

rebase fixes

ac332bb

nagmo-starkware force-pushed the dvir/save_tx_status branch from 48762b3 to ac332bb Compare July 24, 2023 07:04

This was referenced Jul 24, 2023

add v0.4.0 not connected to endpoint #877

Merged

connect v0.4.0 to api endpoint #881

Merged

DvirYo-starkware commented Jul 24, 2023

View reviewed changes

dan-starkware requested changes Jul 24, 2023

View reviewed changes

CR fix.

e51c680

dan-starkware reviewed Jul 24, 2023

View reviewed changes

DvirYo-starkware enabled auto-merge July 24, 2023 10:10

DvirYo-starkware commented Jul 24, 2023

View reviewed changes

Merge main into dvir/save_tx_status

d582610

dan-starkware reviewed Jul 24, 2023

View reviewed changes

dan-starkware approved these changes Jul 24, 2023

View reviewed changes

DvirYo-starkware added this pull request to the merge queue Jul 24, 2023

Merged via the queue into main with commit de7b990 Jul 24, 2023
16 checks passed

DvirYo-starkware deleted the dvir/save_tx_status branch July 24, 2023 11:30

This was referenced Jul 26, 2023

fix v3 spec based on git tag #914

Closed

fix(gateway): v4 handle rejected blocks in tx receipt #915

Merged

nagmo-starkware mentioned this pull request Jul 26, 2023

docs: readme update #916

Merged

9 tasks

github-actions bot locked and limited conversation to collaborators Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dvir/save tx status #888

Dvir/save tx status #888

DvirYo-starkware commented Jul 23, 2023 •

edited by spapinistarkware

Loading

DvirYo-starkware left a comment

nagmo-starkware commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

DvirYo-starkware left a comment

dan-starkware left a comment

dan-starkware left a comment

DvirYo-starkware left a comment

dan-starkware left a comment

dan-starkware left a comment

Dvir/save tx status #888

Dvir/save tx status #888

Conversation

DvirYo-starkware commented Jul 23, 2023 • edited by spapinistarkware Loading

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

DvirYo-starkware left a comment

Choose a reason for hiding this comment

nagmo-starkware commented Jul 24, 2023 • edited Loading

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

DvirYo-starkware left a comment

Choose a reason for hiding this comment

dan-starkware left a comment

Choose a reason for hiding this comment

dan-starkware left a comment

Choose a reason for hiding this comment

DvirYo-starkware left a comment

Choose a reason for hiding this comment

dan-starkware left a comment

Choose a reason for hiding this comment

dan-starkware left a comment

Choose a reason for hiding this comment

DvirYo-starkware commented Jul 23, 2023 •

edited by spapinistarkware

Loading

nagmo-starkware commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading