-
Notifications
You must be signed in to change notification settings - Fork 197
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
Refactor: starknet_estimateMessageFee #2496
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2496 +/- ##
==========================================
+ Coverage 73.65% 73.73% +0.07%
==========================================
Files 137 137
Lines 16659 16641 -18
==========================================
Hits 12271 12271
+ Misses 3526 3511 -15
+ Partials 862 859 -3 ☔ View full report in Codecov by Sentry. |
@@ -65,9 +66,9 @@ func (f FeeEstimate) MarshalJSON() ([]byte, error) { | |||
*****************************************************/ | |||
|
|||
func (h *Handler) EstimateFee(broadcastedTxns []BroadcastedTransaction, | |||
simulationFlags []SimulationFlag, id BlockID, | |||
simulationFlags []rpcv6.SimulationFlag, id BlockID, |
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.
Hmm, it looks like this is a bug actually. We allow SKIP_FEE_CHARGE which isn't allowed by the spec.
The spec distinguishes between SIMULATION_FLAG_FOR_ESTIMATE_FEE (starknet_estFee) and SIMULATION_FLAG (starknet_sumTxns). This seems to be the case for all rpc versions. Not a critical bug, but should probably be fixed
@@ -45,9 +46,9 @@ type FeeEstimate struct { | |||
*****************************************************/ | |||
|
|||
func (h *Handler) EstimateFee(broadcastedTxns []BroadcastedTransaction, | |||
simulationFlags []SimulationFlag, id BlockID, | |||
simulationFlags []rpcv6.SimulationFlag, id BlockID, |
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.
Similar bug here (we accept SKIP_FEE_CHARGE, which isn't allowed by the spec)
LGTM, just need to address the starknet_estFee flags bug |
rpc pkg cleanup according to #2437
Reuse v6
SimulationFlag
andMsgFromL1
types for v7 and v8As i was looking into refactoring
starknet_estimateMessageFee
endpoint, i just found those 2 types to refactor. I don't know if we really want to refactor those, but i opened a PR in case. Let me know if we don't necessarily want to refactor those