Skip to content
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

feat: add raw max fee to TransactionsOptions #1856

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

princeibs
Copy link
Contributor

@princeibs princeibs commented Apr 21, 2024

Closes #1808
Closes DOJ-344

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good first start @princeibs!
The next step is to implement the max fee being applied in the implementation of the TransactionExt trait.

bin/sozo/src/commands/options/transaction.rs Outdated Show resolved Hide resolved
@princeibs princeibs marked this pull request as ready for review April 22, 2024 23:31
Copy link
Contributor

@lambda-0x lambda-0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need to make sure that user shouldn't be able to specify both max_fee_raw and fee_estimate_multiplier at the same time. that can be enforced by clap as well a manual check in the trait implementation

@glihm
Copy link
Collaborator

glihm commented Apr 23, 2024

we also need to make sure that user shouldn't be able to specify both max_fee_raw and fee_estimate_multiplier at the same time. that can be enforced by clap as well a manual check in the trait implementation

Do you know the default behavior of starknet-rs? Because we want to limit that at the command level, no? Or also for the TransactionExt we should enforce that?
Or setting the other to 0 is ok to ensure the behavior is not unexpected?

@lambda-0x
Copy link
Contributor

lambda-0x commented Apr 23, 2024

i was under impression that starknet-rs doesn't allow specifying both at same time but after looking at its code it seems that it will just ignore fee_estimate_multiplier if both of them are specified. so I think we can just use claps to disallow setting both options at same time. And add a comment in implementation that max_fee takes precedence in starknet-rs.

EDIT: sorry @princeibs made this change myself since we want to merge this PR ASAP.

@princeibs
Copy link
Contributor Author

It's all cool @lambda-0x. Is the PR ready to be merged or it needs more any more changes?

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 70.27%. Comparing base (010444f) to head (3f2da67).
Report is 7 commits behind head on main.

Files Patch % Lines
crates/dojo-world/src/utils.rs 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
+ Coverage   70.04%   70.27%   +0.22%     
==========================================
  Files         315      313       -2     
  Lines       35535    35724     +189     
==========================================
+ Hits        24891    25105     +214     
+ Misses      10644    10619      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lambda-0x lambda-0x merged commit 2fabab7 into dojoengine:main Apr 23, 2024
12 checks passed
@lambda-0x
Copy link
Contributor

its done @princeibs, thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sozo] add raw max fee to TransactionsOptions
3 participants