Skip to content
This repository has been archived by the owner on Dec 26, 2024. It is now read-only.

feat(network): add conversion from protobuf transaction outputs to sn api #2055

Conversation

asmaastarkware
Copy link
Contributor

@asmaastarkware asmaastarkware commented May 26, 2024

This change is Reviewable

Copy link

codecov bot commented May 26, 2024

Codecov Report

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

Project coverage is 69.15%. Comparing base (dd3a7c8) to head (6068093).

Files Patch % Lines
crates/papyrus_protobuf/src/converters/receipt.rs 0.00% 93 Missing ⚠️
...tes/papyrus_protobuf/src/converters/transaction.rs 0.00% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2055      +/-   ##
==========================================
- Coverage   69.63%   69.15%   -0.49%     
==========================================
  Files         131      131              
  Lines       17220    17340     +120     
  Branches    17220    17340     +120     
==========================================
  Hits        11992    11992              
- Misses       3904     4024     +120     
  Partials     1324     1324              

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

@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from 61d44bb to d06c5bf Compare May 26, 2024 10:17
@asmaastarkware asmaastarkware force-pushed the asmaa/receipt_proto_to_sn_api branch from e3a4136 to 24459b6 Compare May 26, 2024 13:36
@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from d06c5bf to db6b510 Compare May 26, 2024 13:59
@asmaastarkware asmaastarkware force-pushed the asmaa/receipt_proto_to_sn_api branch from 24459b6 to b4feeb4 Compare May 27, 2024 07:49
@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from db6b510 to 38fcac0 Compare May 27, 2024 07:56
@asmaastarkware asmaastarkware force-pushed the asmaa/receipt_proto_to_sn_api branch from b4feeb4 to 8ed7fa2 Compare May 30, 2024 09:04
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @dan-starkware)


crates/papyrus_protobuf/src/converters/receipt.rs line 107 at r2 (raw file):

}

// The price_unit will be updated in the caller function

Change this to:
// The returned price_unit isn't correct. It can be fixed by calling set_price_unit_based_on_transaction
In general, comments should be in the scope of the function and not the entire crate
Same in other places


crates/papyrus_protobuf/src/converters/receipt.rs line 452 at r2 (raw file):

}

fn create_receipt_common_from_txn_output_fields(

Add proto to this name somewhere
create_proto_receipt_common_from_txn_output_fields


crates/papyrus_protobuf/src/converters/transaction.rs line 1122 at r2 (raw file):

        _ => return,
    };
    if let Some(ref mut receipt_type) = receipt.r#type {

use let-else to remove one indentation to all the code below


crates/papyrus_protobuf/src/converters/transaction.rs line 1125 at r2 (raw file):

        match receipt_type {
            protobuf::receipt::Type::Invoke(invoke) => {
                if let Some(ref mut common) = invoke.common {

return here the reference to common and in all other transactions and then, in a single place, set the price unit

Base automatically changed from asmaa/receipt_proto_to_sn_api to main May 30, 2024 09:16
@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from 38fcac0 to 45b99ec Compare May 30, 2024 09:37
Copy link
Contributor Author

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dan-starkware and @ShahakShama)


crates/papyrus_protobuf/src/converters/receipt.rs line 107 at r2 (raw file):

Previously, ShahakShama wrote…

Change this to:
// The returned price_unit isn't correct. It can be fixed by calling set_price_unit_based_on_transaction
In general, comments should be in the scope of the function and not the entire crate
Same in other places

Done.


crates/papyrus_protobuf/src/converters/receipt.rs line 452 at r2 (raw file):

Previously, ShahakShama wrote…

Add proto to this name somewhere
create_proto_receipt_common_from_txn_output_fields

Done.


crates/papyrus_protobuf/src/converters/transaction.rs line 1122 at r2 (raw file):

Previously, ShahakShama wrote…

use let-else to remove one indentation to all the code below

Done.


crates/papyrus_protobuf/src/converters/transaction.rs line 1125 at r2 (raw file):

Previously, ShahakShama wrote…

return here the reference to common and in all other transactions and then, in a single place, set the price unit

Done.

@asmaastarkware asmaastarkware enabled auto-merge May 30, 2024 09:38
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @dan-starkware)


crates/papyrus_protobuf/src/converters/receipt.rs line 115 at r3 (raw file):

            value.execution_status,
        );
        // The returned price_unit isn't correct.

Move this to the start of the function. And change it to docstring (3 slashes)
Same everywhere

@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from 45b99ec to dae7be4 Compare May 30, 2024 10:04
Copy link
Contributor Author

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

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

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


crates/papyrus_protobuf/src/converters/receipt.rs line 115 at r3 (raw file):

Previously, ShahakShama wrote…

Move this to the start of the function. And change it to docstring (3 slashes)
Same everywhere

Done.

@asmaastarkware asmaastarkware force-pushed the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch from dae7be4 to 6068093 Compare May 30, 2024 10:05
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)

@asmaastarkware asmaastarkware added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit 065af83 May 30, 2024
19 checks passed
@asmaastarkware asmaastarkware deleted the asmaa/add_conversions_from_proto_transaction_outputs_to_sn_api branch May 30, 2024 10:16
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants