-
Notifications
You must be signed in to change notification settings - Fork 29
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 {
c984a6b
to
52c3ee5
Compare
There was a problem hiding this 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 withinTransaction
/BroadcastTransaction
. I understand they may only be used byTransaction
, 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.
There was a problem hiding this 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)
There was a problem hiding this 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;
This change is