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

add broadcasted transaction #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShahakShama
Copy link
Collaborator

@ShahakShama ShahakShama commented May 26, 2024

This change is Reviewable

Copy link
Collaborator

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ittaysw and @ShahakShama)


p2p/proto/transaction.proto line 127 at r1 (raw file):

        Felt252 entry_point_selector = 4;
        repeated Felt252 calldata = 5;
    }

I prefer to move these outside of Transaction like the other messages, so that only Declare messages are defined within Transaction/BroadcastTransaction. I understand they may only be used by Transaction, but their type definition isn't tied to being inside of one versus the other like with Declare.

Code quote:

    message Deploy {
        Hash class_hash = 1;
        Felt252 address_salt = 2;
        repeated Felt252 calldata = 3;
        uint32 version = 4;
    }

    message InvokeV0 {
        Felt252 max_fee = 1;
        AccountSignature signature = 2;
        Address address = 3;
        Felt252 entry_point_selector = 4;
        repeated Felt252 calldata = 5;
    }

p2p/proto/transaction.proto line 173 at r1 (raw file):

// It also contains the entire class for Declare.
// Useful for consensus and mempool.
message BroadcastedTransaction {

I thought it was Broadcast, not Broadcasted?

Code quote:

BroadcastedTransaction

p2p/proto/transaction.proto line 182 at r1 (raw file):

    }

    message BroadcastedDeclareV3 {

Can we just call them Declare? They are defined within the namespace of BroadcastedTransaction already.

Code quote:

    message BroadcastedDeclareV2 {
        Address sender = 1;
        Felt252 max_fee = 2;
        AccountSignature signature = 3;
        Cairo1Class class = 4;
        Felt252 nonce = 5;
    }

    message BroadcastedDeclareV3 {

@ShahakShama ShahakShama force-pushed the shahak/broadcasted_transaction branch from c984a6b to 52c3ee5 Compare May 26, 2024 13:36
Copy link
Collaborator Author

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ariel, @ittaysw, and @matan-starkware)


p2p/proto/transaction.proto line 127 at r1 (raw file):

Previously, matan-starkware wrote…

I prefer to move these outside of Transaction like the other messages, so that only Declare messages are defined within Transaction/BroadcastTransaction. I understand they may only be used by Transaction, but their type definition isn't tied to being inside of one versus the other like with Declare.

Done.


p2p/proto/transaction.proto line 173 at r1 (raw file):

Previously, matan-starkware wrote…

I thought it was Broadcast, not Broadcasted?

It's a transaction that was broadcasted. We can ask @ariel if he has a better name


p2p/proto/transaction.proto line 182 at r1 (raw file):

Previously, matan-starkware wrote…

Can we just call them Declare? They are defined within the namespace of BroadcastedTransaction already.

Done.

Copy link
Collaborator

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ariel and @ittaysw)

Copy link
Collaborator

@matan-starkware matan-starkware 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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ariel, @ittaysw, and @ShahakShama)


p2p/proto/transaction.proto line 133 at r2 (raw file):

        DeclareV1 declare_v1 = 2;
        DeclareV2 declare_v2 = 3;
        DeclareV3 declare_v3 = 4;

These should be defined inside of message Transaction

Code quote:

        DeclareV0 declare_v0 = 1;
        DeclareV1 declare_v1 = 2;
        DeclareV2 declare_v2 = 3;
        DeclareV3 declare_v3 = 4;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants