-
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 deploy account as wrapper #92
chore: refactor deploy account as wrapper #92
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
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 9 files reviewed, all discussions resolved (waiting on @dafnamatsry and @MohammadNassar1)
crates/blockifier/src/transaction/account_transaction.rs
line 77 at r2 (raw file):
match self { Self::Declare(tx) => tx.tx.version(), Self::DeployAccount(tx) => tx.tx.tx.version(),
Yuck.
Code quote:
Self::DeployAccount(tx) => tx.tx.tx.version(),
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 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)
crates/blockifier/src/transaction/account_transaction.rs
line 77 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Yuck.
OMG!
crates/blockifier/src/transaction/transactions.rs
line 296 at r2 (raw file):
// Indicates the presence of the only_query bit in the version. pub only_query: bool, }
Can we use Deref
here?
So, we don't need to add tx
in all the place?
Code quote:
pub struct DeployAccountTransaction {
pub tx: starknet_api::executable_transaction::DeployAccountTransaction,
// Indicates the presence of the only_query bit in the version.
pub only_query: bool,
}
Previously, MohammadNassar1 (mohammad-starkware) wrote…
I am unfamiliar with this design pattern ( I assume this is what you mean). |
Previously, ArniStarkware (Arnon Hod) wrote…
Upon further reading - I think I understand. |
a491474
to
e90a275
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 10 files reviewed, all discussions resolved (waiting on @dafnamatsry and @MohammadNassar1)
crates/blockifier/src/transaction/transactions.rs
line 296 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Upon further reading - I think I understand.
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 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)
crates/blockifier/src/transaction/transactions.rs
line 296 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Done.
Nice!
crates/blockifier/src/transaction/transactions.rs
line 383 at r3 (raw file):
}; match &self.tx.tx {
Deref cannot work here?
Code quote:
&self.tx.tx
crates/starknet_api/src/executable_transaction.rs
line 71 at r3 (raw file):
} impl std::ops::Deref for DeployAccountTransaction {
Suggestion:
Deref
e90a275
to
8d26abe
Compare
8d26abe
to
100b093
Compare
No longer relevant. |
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 10 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @MohammadNassar1)
crates/blockifier/src/transaction/transactions.rs
line 383 at r3 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Deref cannot work here?
Used a different approch.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 76.81% 76.83% +0.02%
==========================================
Files 312 312
Lines 34446 34480 +34
Branches 34446 34480 +34
==========================================
+ Hits 26458 26494 +36
+ Misses 5702 5701 -1
+ Partials 2286 2285 -1 ☔ View full report in Codecov by Sentry. |
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 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)
crates/starknet_api/src/executable_transaction.rs
line 106 at r4 (raw file):
pub fn contract_address(&self) -> ContractAddress { self.contract_address }
What do you think about using a macro instead?
Code quote:
pub fn tx(&self) -> &crate::transaction::DeployAccountTransaction {
&self.tx
}
pub fn tx_hash(&self) -> TransactionHash {
self.tx_hash
}
pub fn contract_address(&self) -> ContractAddress {
self.contract_address
}
100b093
to
fe6f0a3
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: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @MohammadNassar1)
crates/starknet_api/src/executable_transaction.rs
line 106 at r4 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
What do you think about using a macro instead?
Done.
I did not create a macro just for the ref for tx
, as this is just one line. (It will repeat 3 times eventually - we can add it then if we wish).
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 r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)
dd0da5e
to
bdc47af
Compare
cfb3c39
to
78b45af
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 10 files at r4, 5 of 5 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
78b45af
to
0fe0829
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
|
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
IntrnalDeployAccountTransaction
and ofDeployAccountTransaction
(on the blockifier) is duplicated.What is the new behavior?
DeployAccountTransaction
as a wrapper ofIntrnalDeployAccountTransaction
while keeping the implementation.Does this introduce a breaking change?
Other information
This change is