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

[sozo] expose fee multiplier for sozo migrate #1801

Closed
glihm opened this issue Apr 9, 2024 · 4 comments
Closed

[sozo] expose fee multiplier for sozo migrate #1801

glihm opened this issue Apr 9, 2024 · 4 comments
Labels
good first issue Good for newcomers sozo

Comments

@glihm
Copy link
Collaborator

glihm commented Apr 9, 2024

Is your feature request related to a problem? Please describe.
sozo migrate command does have a fee multiplier in the code, but it's not exposed to the user.
This can cause trouble when migrating to a Katana with fees activated or a Starknet network.

Describe the solution you'd like
In a first step, this PR should expose the fee multiplier to the user.
In a second PR, we may want to use the raw fee or fee multiplier, introduced in #1601.

Additional context
The apply_diff function has a TxConfig which contains a fee multiplier field.
It is set to None. The modification should make this field coming from the CLI.

pub async fn apply_diff<P, S>(
ws: &Workspace<'_>,
account: &SingleOwnerAccount<P, S>,
txn_config: Option<TxConfig>,
strategy: &mut MigrationStrategy,
) -> Result<MigrationOutput>

@glihm glihm added good first issue Good for newcomers sozo labels Apr 9, 2024
@kariy
Copy link
Member

kariy commented Apr 9, 2024

Fee multiplier already exist in the TransactionOptions:

pub struct TransactionOptions {
#[arg(long)]
#[arg(value_name = "MULTIPLIER")]
#[arg(help = "The multiplier to use for the fee estimate.")]
#[arg(long_help = "The multiplier to use for the fee estimate. This value will be used on \
the estimated fee which will be used as the max fee for the transaction. \
(max_fee = estimated_fee * multiplier)")]
pub fee_estimate_multiplier: Option<f64>,

iirc sozo migrate used to accept the tx config along with the multiplier.

@glihm
Copy link
Collaborator Author

glihm commented Apr 9, 2024

Fee multiplier already exist in the TransactionOptions:

pub struct TransactionOptions {
#[arg(long)]
#[arg(value_name = "MULTIPLIER")]
#[arg(help = "The multiplier to use for the fee estimate.")]
#[arg(long_help = "The multiplier to use for the fee estimate. This value will be used on \
the estimated fee which will be used as the max fee for the transaction. \
(max_fee = estimated_fee * multiplier)")]
pub fee_estimate_multiplier: Option<f64>,

iirc sozo migrate used to accept the tx config along with the multiplier.

Yeah good point, I think it was a regression here. I've added it back into #1802.

From the conversation we had in the discord with the community, I think we should add in the TransactionsOptions a raw max fee to ensure it can be set independently of the node capabilities to estimate fee. Sounds good to you @kariy?

Will open an issue for this next task if you agree.

@kariy
Copy link
Member

kariy commented Apr 9, 2024

From the conversation we had in the discord with the community, I think we should add in the TransactionsOptions a raw max fee to ensure it can be set independently of the node capabilities to estimate fee. Sounds good to you @kariy?

Will open an issue for this next task if you agree.

Yeah, we thought about adding this before when we're adding the multiplier flag. Sounds good to me

@glihm
Copy link
Collaborator Author

glihm commented Apr 9, 2024

Will close in this one is favor of #1808.

@glihm glihm closed this as completed Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers sozo
Projects
None yet
Development

No branches or pull requests

2 participants