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

write integration test for taker cli app. #244

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KnowWhoami
Copy link
Collaborator

@KnowWhoami KnowWhoami commented Aug 23, 2024

fixes #207

Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

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

Approach Ack.

Needs restructuring.

tests/taker_cli.rs Outdated Show resolved Hide resolved
tests/taker_cli.rs Outdated Show resolved Hide resolved
tests/taker_cli.rs Outdated Show resolved Hide resolved
@KnowWhoami KnowWhoami force-pushed the taker_test branch 2 times, most recently from 93f0a76 to 886fb5d Compare September 10, 2024 07:27
@KnowWhoami KnowWhoami marked this pull request as ready for review September 10, 2024 07:28
Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

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

Approach Ack.

Some more fixes.

src/bin/taker.rs Outdated
Comment on lines 85 to 91
/// Recipient address
#[clap(name = "address")]
address: String,
/// Amount to be sent (in satoshis)
#[clap(name = "amount")]
amount: u64,

Choose a reason for hiding this comment

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

We need a fee in this structure itself. That's not the same as the coinswap fee. Then use the fee used here to create the tx.

src/bin/taker.rs Outdated
Comment on lines 180 to 181
// Assuming fee=fee_rate
// TODO: find a way to derive fee.

Choose a reason for hiding this comment

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

No fee and fee_rate is different. Doing fee_rate is difficult, because fee = fee_rate * tx_weight. And we don't know the tx_weight before signing the transaction. But we need the fee to create the outputs before signing the tx.

So it becomes a chicken and egg style problem.

One way to solve this is to pre-estimate the tx_weight, which is possible because its always single-sig p2wpkh type, which has a constant size of tx input.

This will get solved by BDK as it internally knows how to estimate input size from the descriptor.

For our purpose now having just the fee is will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Whenever we create any sort of tx like funding, contract tx -> we take fee-rate in function parameters

pub fn create_senders_contract_tx(

fn create_funding_txes_random_amounts(

  • Right now , we do not calculate fee of a tx , so we just assume fee= fee_rate.

  • So we should also do the same for spend_from_wallet api which is currently taking fee as parameter.

    pub fn spend_from_wallet(

  • IMO, it seems irrational from user perspective to give fee as argument while calling send-to-addresss .

tests/taker_cli.rs Outdated Show resolved Hide resolved
src/bin/taker.rs Outdated Show resolved Hide resolved
@KnowWhoami KnowWhoami force-pushed the taker_test branch 3 times, most recently from f28e54c to 87d26cb Compare September 17, 2024 10:37
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.

Implement Integration Test for taker-cli
2 participants