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(cli): feeChange commands #1622

Closed
wants to merge 4 commits into from

Conversation

quasystaty1
Copy link
Contributor

@quasystaty1 quasystaty1 commented Oct 3, 2024

Summary

Adds FeeChangeAction to the cli as a sudo command

Background

The ability to change fees required by different actions is not supported by the cli, should be added as part of adding all sequencer actions as cli commands.

Changes

  • adds fee-change sudo Subcommand with all fee change actions

Testing

There will be a follow-up PR to add CLI test after the ongoing test refactor is completed.

Related Issues

part of #1474
closes #1615

@quasystaty1 quasystaty1 marked this pull request as ready for review October 3, 2024 21:06
@quasystaty1 quasystaty1 requested a review from a team as a code owner October 3, 2024 21:06
@quasystaty1 quasystaty1 requested a review from Fraser999 October 3, 2024 21:06
inner: ArgsInner,
}

impl SequenceByteCostMul {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to type out the whole name for this type. SequenceByteCostMultiplier

}

#[derive(Clone, Debug, clap::Args)]
struct BridgeLockByteCostMul {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use whole name.

inner: ArgsInner,
}

impl BridgeLockByteCostMul {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use whole name.

Copy link
Contributor

@sambukowski sambukowski left a comment

Choose a reason for hiding this comment

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

LGTM. Just nits on struct naming.

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

Blocking here on #1647 because it's going to change structure here quite a bit.

@sambukowski sambukowski added the cli pertaining to the cli label Oct 9, 2024
@quasystaty1 quasystaty1 force-pushed the quasystaty1/ENG-901/cli/change-fee-command branch from f447a92 to b26657f Compare October 24, 2024 15:53
Copy link
Contributor

@ethanoroshiba ethanoroshiba left a comment

Choose a reason for hiding this comment

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

Aside from some typos, LGTM.

SubCommand::BridgeUnlockFee(bridge_unlock) => bridge_unlock.run().await,
SubCommand::BridgeSudoChangeFee(bridge_sudo_change) => bridge_sudo_change.run().await,
SubCommand::Ics20WithdrawalFee(ics20_withdrawal) => ics20_withdrawal.run().await,
SubCommand::IbcRelaeyFee(ibc_relay) => ibc_relay.run().await,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SubCommand::IbcRelaeyFee(ibc_relay) => ibc_relay.run().await,
SubCommand::IbcRelayFee(ibc_relay) => ibc_relay.run().await,

#[allow(clippy::enum_variant_names)]
#[derive(Debug, Subcommand)]
enum SubCommand {
/// Chnage Transfer Fee
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Chnage Transfer Fee
/// Change Transfer Fee

TransferFee(Transfer),
/// Change Init Bridge Account Fee
InitBridgeFee(BridgeInit),
/// Change Sequence Fee
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to call this Rollup Data Submission Fee? I'm not opinionated either way, but maybe worth considering.

/// Change ICS20 Withdrawal Fee
Ics20WithdrawalFee(Ics20Withdrawal),
/// Change IBC Relay Fee
IbcRelaeyFee(IbcRelay),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IbcRelaeyFee(IbcRelay),
IbcRelayFee(IbcRelay),

@quasystaty1 quasystaty1 force-pushed the quasystaty1/ENG-901/cli/change-fee-command branch from 2111b0d to a3cec30 Compare November 4, 2024 16:51
Copy link
Contributor

@ethanoroshiba ethanoroshiba left a comment

Choose a reason for hiding this comment

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

LGTM

@joroshiba
Copy link
Member

This PR is stale because it has been open 45 days with no activity. Remove stale label or this PR will be
closed in 7 days.

@joroshiba
Copy link
Member

This PR was closed because it has been stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli pertaining to the cli closed-stale stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(cli): all feeChange commands
4 participants