citadel-tech / coinswap

Functioning, minimal-viable binaries and libraries to perform a trustless, p2p Maxwell-Belcher Coinswap Protocol
https://gist.github.com/chris-belcher/9144bd57a91c194e332fb5ca371d0964
Other
73 stars 46 forks source link

refactor `get_next_internal_address` to give only 1 address #245

Closed KnowWhoami closed 2 months ago

KnowWhoami commented 2 months ago

Edit: see https://github.com/citadel-tech/coinswap/pull/245#discussion_r1732905255 Fixes #242

KnowWhoami commented 2 months ago

Add an assert to check if the count is zero or not.

We don't require that since derive_address rpc call itself take care of it. I call get_next_internal_address with count =0 and got this error:

image

KnowWhoami commented 2 months ago

Should I write doc comments to mention this?

Shourya742 commented 2 months ago

Add an assert to check if the count is zero or not.

We don't require that since derive_address rpc call itself take care of it. I call get_next_internal_address with count =0 and got this error:

image

Not exactly. We need to capture deterministic errors in the error propagation chain as early as possible. In this case, capturing the error is especially important because we're making a network call, and we want to avoid intentionally sending a buggy payload. What you're suggesting isn't a good practice.

Now that I think about it, this must be why Belcher didn’t use this approach in the first place—to avoid errors in this situation. Given how vast the address space is, we’re unlikely to exhaust it. It might be better to leave it as is and simply document it.

KnowWhoami commented 2 months ago

https://github.com/citadel-tech/coinswap/blob/73e17a80ac10d42b97966430d745d1bf90155563/src/wallet/api.rs#L923

https://github.com/citadel-tech/coinswap/blob/73e17a80ac10d42b97966430d745d1bf90155563/src/wallet/api.rs#L908

mojoX911 commented 2 months ago

Parked for BDK.