-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore: refactor invoke as wrapper #93
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ArniStarkware and the rest of your teammates on Graphite |
ff1164f
to
a491474
Compare
3f5ec5c
to
decc9f8
Compare
a491474
to
e90a275
Compare
decc9f8
to
91422c2
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.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)
crates/blockifier/src/transaction/transactions.rs
line 495 at r3 (raw file):
let common_fields = CommonAccountFields { transaction_hash: self.tx.tx_hash, version: self.tx.tx.version(),
and the same for the other fields pls
Suggestion:
self.tx.version()
e90a275
to
8d26abe
Compare
91422c2
to
01f2f9a
Compare
8d26abe
to
100b093
Compare
01f2f9a
to
a5eda93
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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @MohammadNassar1)
crates/blockifier/src/transaction/transactions.rs
line 495 at r3 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
and the same for the other fields pls
Done.
100b093
to
fe6f0a3
Compare
a5eda93
to
4b65ebe
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.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)
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 3 files at r1, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
fe6f0a3
to
dd0da5e
Compare
4b65ebe
to
62b16c3
Compare
dd0da5e
to
bdc47af
Compare
62b16c3
to
747086b
Compare
Please review this change from the base commit. Code quote: AccountTransaction::Invoke(tx) => match &tx.tx() { |
bdc47af
to
cfb3c39
Compare
747086b
to
1a1fc1b
Compare
cfb3c39
to
78b45af
Compare
1a1fc1b
to
a553ba4
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.
Reviewed 3 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
78b45af
to
0fe0829
Compare
a553ba4
to
f23134e
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.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
Merge activity
|
f23134e
to
a9d464a
Compare
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
For internal deploy account transaction, the data is inseparable from the implementation.
The data of
IntrnalInvokeTransaction
and ofInvokeTransaction
(on the blockifier) is duplicated.What is the new behavior?
InvokeTransaction
as a wrapper ofIntrnalInvokeTransaction
while keeping the implementation.Does this introduce a breaking change?
Other information
This change is