Skip to content

Conversation

@KnowWhoami
Copy link

@KnowWhoami KnowWhoami commented Dec 7, 2024

This pr aims to:

  • remove rpc_port from TakerConfig as Taker is just a application which doesn't require a rpc_port.
    but it requires port just for having hidden services while running on tor for maintaining privacy so Documented that point.
  • move all do-coinswap related arguments inside the command.
  • remove sync-offerbook call as this syncing process is done while performing coinswap and thus we don't require to have a seperate command for that.
  • create list-offerbook call to see all the offers which taker has in his offerbook.

Work left to be done here ( after merging of #305 , #311 ) :

  • There is a potential bug while calling do-coinswap call -> I am planning to include that bug fix in this pr itself
  • I'll also extend the taker-cli IT to test the remaining commands

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.

Ack. few comments.

Comment on lines 91 to 96
/// Sets the fee-rate.
#[clap(name = "fee_rate")]
fee_rate: u64,
/// Sets the required on-chain confirmations.
#[clap(name = "required_confirms")] // TODO: Should we have default as 1?
required_confirms: u64,

Choose a reason for hiding this comment

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

I think its better to hardcoded these to some default values, at least in the App. As these can be gamed to make malicious swap requests.

yes a default of required_confirms= 1 is good enough. If it causes reorg problems we will change to 2/3 later. Too high value will make a swap process very slow.

Comment on lines +103 to +102
/// Sets the transaction count.
#[clap(name = "tx_count", default_value = "3")]
tx_count: u32,

Choose a reason for hiding this comment

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

Lets make default to 1. Also this need a good explainer doc. Setting it's value n>1 will create "split-swaps". Which takes 1 or more utxos, and gives n new utxos back.

Setting it to 1 simulates a "merge_swap`, which takes 1 or more utxos, and only give 1 new utxo back.

Splits swaps are more costly than merge swaps. A 3tx swap will be about 3 times more costly than a 1tx swap. This will need to be improved later to reduce coinswap cost. Its unreasonably high right now.

 - Document the need of `port` for establishing tor hidden service for `Taker`.
 - Make `do-coinswap` a subcommand.
 - Remove `sync-offerbook`  & create `list-offerbook` command
@mojoX911
Copy link

mojoX911 commented Jan 1, 2025

will be closed by #351

@mojoX911 mojoX911 closed this Jan 1, 2025
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.

2 participants