search

dojoengine / dojo

Dojo is a toolchain for building provable games and autonomous worlds with Cairo
https://dojoengine.org
Apache License 2.0
386 stars 147 forks source link

[sozo] expose fee multiplier for `sozo migrate` #1801

Closed glihm closed 2 months ago

glihm commented 2 months ago

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. https://github.com/dojoengine/dojo/blob/f0637f928470217fccd60fe15b1b9c6fefd5c984/crates/sozo/ops/src/migration/mod.rs#L275-L280

kariy commented 2 months ago

Fee multiplier already exist in the TransactionOptions:

https://github.com/dojoengine/dojo/blob/64fa43c6551bc5fe4eff2676723f066ccb4e12cd/bin/sozo/src/commands/options/transaction.rs#L6-L13

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

glihm commented 2 months ago

Fee multiplier already exist in the TransactionOptions:

https://github.com/dojoengine/dojo/blob/64fa43c6551bc5fe4eff2676723f066ccb4e12cd/bin/sozo/src/commands/options/transaction.rs#L6-L13

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 commented 2 months ago

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 commented 2 months ago

Will close in this one is favor of #1808.