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

Fix channel amount and fee issues #69

Merged
merged 35 commits into from
Jun 26, 2024

Conversation

chenyukang
Copy link
Collaborator

@chenyukang chenyukang commented Jun 14, 2024

This PR contains several changes:

  • Add several default configuration number for minimal ckb amount
  • Change ckb amount and fee related fields from u128 to u64
  • Add commitment_fee_rate in open channel rpc
  • Use fee_rate instead of fee in the shutdown command, and verify shutdown fee is valid when handing shutdown command
  • Introduce local_reserve_ckb_amount and remote_reserve_ckb_amount to make sure we can always build valid commitment tx or shutdown tx.
  • Add funding_fee_rate option to specify and calculate the fee in funding transaction.

@chenyukang chenyukang changed the title [WIP] Fix channel amount issues [WIP] Fix channel amount and fee issues Jun 14, 2024
@chenyukang chenyukang force-pushed the fix-channel-amount-issues branch 4 times, most recently from e1147ad to 5b09c5a Compare June 20, 2024 02:41
@chenyukang chenyukang changed the title [WIP] Fix channel amount and fee issues Fix channel amount and fee issues Jun 21, 2024
src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
@chenyukang chenyukang force-pushed the fix-channel-amount-issues branch 6 times, most recently from 8bb4b54 to b39b2de Compare June 25, 2024 00:05
src/ckb/channel.rs Outdated Show resolved Hide resolved
@@ -180,6 +181,7 @@ pub enum NetworkActorEvent {
Option<Script>,
u64,
u64,
u64,
Copy link
Member

Choose a reason for hiding this comment

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

To refactor later

Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

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

Reviewed

src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/network.rs Outdated Show resolved Hide resolved
src/ckb/network.rs Outdated Show resolved Hide resolved
src/ckb/schema/cfn.mol Outdated Show resolved Hide resolved
// The commitment fee rate is used to calculate the fee for the commitment transactions.
// The side who starting a shutdown command need to pay at least this fee for building shutdown transaction,
// the other side may choose to pay less fee rate if the current fee is enough.
pub commitment_fee_rate: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name commitment_fee_rate and the comment is not consistent. Why is commitment_fee_rate named commitment_fee_rate although it is used only in the shutdown transaction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

commitment_fee_rate will also be used in commitment transaction (I will add it later, I haven't determined which party pay the fee or both parties pay the fee), we treat shutdown transaction is a special commitment transaction.

src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
src/ckb/channel.rs Outdated Show resolved Hide resolved
@@ -152,11 +129,17 @@ impl TxBuilder for FundingTxBuilder {
None => packed::Transaction::default().as_advanced_builder(),
};

// set a placeholder_witness for calculating transaction fee according to transaction size
let placeholder_witness = packed::WitnessArgs::new_builder()
.lock(Some(molecule::bytes::Bytes::from(vec![0u8; 170])).pack())
Copy link
Member

Choose a reason for hiding this comment

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

How did this hard-coded 170 come about?

@quake quake merged commit a46d08c into nervosnetwork:main Jun 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants