-
Notifications
You must be signed in to change notification settings - Fork 292
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
Remove hardcoded values in estimate_fee test #1526
base: main
Are you sure you want to change the base?
Remove hardcoded values in estimate_fee test #1526
Conversation
starknet-rpc-test/estimate_fee.rs
Outdated
let invoke_transaction_2 = | ||
BroadcastedTransaction::Invoke(BroadcastedInvokeTransaction { nonce: FieldElement::ONE, ..tx }); | ||
|
||
let simulations = rpc |
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.
can we execute it instead of doing the simulation? i think that would be a better test, wdyt?
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.
@apoorvsadana Thank you for your comment. Could you please clarify how to check the actual fee after execution? Additionally, does "executing it" refer to calling JsonRpcClient.add_invoke_transaction? Thanks
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.
You can fetch the receipt to get the actual_fee. Yes, you need to call add_invoke_transaction
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.
@apoorvsadana, Thank you for your help. do you know how long we should wait until MaybePendingTransactionReceipt
gets the Receipt
value?
In my local machine, waiting for 20 seconds works fine, estimate_fee equals actual_fee(0xf0).
However, in the rpc-tests, it fails. After I changed wait time to 200 seconds, it still failed.
In another case, after adding PendingReceipt to match arm, it fails in rpc-tests because PendingReceipt.actual_fee(0xdc) is slightly smaller than Receipt.actual_fee(0xf0).
It looks like there is no validate, the fee should be 0xd2(210), if validate is skipped, the fee should be 0xdc(220), with signature, the fee should be 0xf0(240).
…into 1368_fee_test
… fee with actual fee
blocked by #1573 We are going to wait a bit before rebasing and merging this one, okay? |
@apoorvsadana Could you help to review this PR? Thanks |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1526 +/- ##
==========================================
- Coverage 40.71% 31.22% -9.49%
==========================================
Files 96 89 -7
Lines 11069 9190 -1879
Branches 11069 9190 -1879
==========================================
- Hits 4507 2870 -1637
+ Misses 6050 5855 -195
+ Partials 512 465 -47 ☔ View full report in Codecov by Sentry. |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the current behavior?
Resolves: #1368
What is the new behavior?
Replace hardcoded value in estimate_fee test with actual fee
Does this introduce a breaking change?
Other information