feat: initial implementation of walletv2 #6429

joschisan · 2024-11-21T06:53:14Z

No description provided.

dpc · 2024-12-17T18:03:42Z

devimint/src/federation/config.rs

@@ -101,6 +101,24 @@ pub fn attach_default_module_init_params(
        );
    }

+    if fedimintd_version >= &VERSION_0_5_0_ALPHA
+        && !is_env_var_set(FM_DEVIMINT_DISABLE_MODULE_LNV2_ENV)


s/LNV2/WALLETV2/?

dpc · 2024-12-17T18:06:54Z

fedimint-testing/src/btc/mock.rs

@@ -65,7 +68,7 @@ struct FakeBitcoinTestInner {
    /// Simulates the merkle tree proofs
    proofs: BTreeMap<Txid, TxOutProof>,
    /// Simulates the script history
-    scripts: BTreeMap<ScriptBuf, Vec<Transaction>>,
+    scripts: BTreeMap<ScriptBuf, Vec<(Transaction, u64)>>,


Oh, it's a block height? Hard to tell from looking at it. Either a comment, or maybe

type BlockHeight = u64; and scripts: BTreeMap<ScriptBuf, Vec<(Transaction, BlockHeight)>>?

dpc · 2024-12-17T18:11:43Z

fedimint-testing/src/btc/mock.rs

-            .scripts
-            .insert(address.script_pubkey(), vec![transaction.clone()]);
+
+        if let Some(txs) = inner.scripts.get_mut(&address.script_pubkey()) {


Nit: I think it can be done cleaner with inner.scripts.entry(address.script_pubkey()).or_default().push((transaction.clone(), block_height));

dpc · 2024-12-17T18:20:09Z

fedimint-testing/src/btc/mock.rs

+        Ok(inner
+            .scripts
+            .get(script)
+            .cloned()


Super nit:

Ok(inner .scripts .get(script) .into_iter() .flat_map(|v| v.iter().map(|entry| entry.0.clone())) .collect())

will save whole one allocation (of the Vec, individual Transactions will still get cloned). :D

dpc · 2024-12-17T18:21:32Z

fedimintd/src/fedimintd.rs

@@ -368,6 +368,27 @@ impl Fedimintd {
            s
        };

+        let s = if is_env_var_set(FM_ENABLE_MODULE_LNV2_ENV) {


WALLETV2?

dpc · 2024-12-17T18:22:54Z

misc/git-hooks/pre-commit

@@ -68,7 +68,7 @@ function check_check_forbidden_dependencies() {
      >&2 echo "fedimint-server/Cargo.toml must not depend on modules"
      return 1
    fi
-    if grep -E "(fedimint-mint|fedimint-wallet|fedimint-ln-(server|client))" fedimint-testing/Cargo.toml >&2 ; then
+    if grep -E "(fedimint-mint|fedimint-wallet-|fedimint-ln-(server|client))" fedimint-testing/Cargo.toml >&2 ; then


Blocking: fedimint-testing must not depend on modules because it creates tons of extra dependencies and ruins the architecture. Whatever needs it should go to some other crate or something.

Ehhh... OK, let's allow -commons. Someone is going to put a big fat dependency in one of the -commons eventually and ruin everything, but oh well.

AFAICT, the correct one should be: ((fedimint-mint|fedimint-wallet|fedimint-ln)-(server|client))

OK, let's just sort it out in another PR: #6573 @joschisan

dpc · 2024-12-17T20:05:56Z

modules/fedimint-walletv2-client/src/lib.rs

+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct SendMeta {


Do we have a consensus on Send/Receive nomenclature? I guess Send == Pegout?

dpc · 2024-12-17T20:07:44Z

modules/fedimint-walletv2-client/src/lib.rs

+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct UnspentDeposit {


Unspent == Unclaimed? And here it's a Deposit, but above "Receive"? :D

DepositMeta seems to make more sense than ReceiveMeta in that case. The operations can be "send" and "receive" but when talking about nouns, "deposit" and "withdrawal" sound better, IMO.

Writting docstring for these would clarify any doubts and help with the naming.

dpc · 2024-12-17T20:09:55Z

modules/fedimint-walletv2-client/src/lib.rs

+}
+
+impl Context for WalletClientContext {
+    const KIND: Option<ModuleKind> = None;


None?! Shouldn't this be Some(KIND)? Other modules set it.

dpc · 2024-12-17T20:38:22Z

modules/fedimint-walletv2-client/src/lib.rs

+    pub esplora_connection: DynEsploraConnection,
+}
+
+impl Default for WalletClientInit {


impl Default conceptually "making connections" is a bit odd, even if "under the hood" no actual connection is yet made.

dpc · 2024-12-17T20:41:09Z

modules/fedimint-walletv2-common/src/esplora_api.rs

+}
+
+#[derive(Debug, Clone)]
+pub struct RealEsploraConnection;


We might want to stuff reqwest::Client here to reuse it.

The Client holds a connection pool internally, so it is advised that you create one and reuse it.

https://docs.rs/reqwest/latest/reqwest/struct.Client.html

dpc · 2024-12-17T20:43:49Z

modules/fedimint-walletv2-client/src/lib.rs

+    /// Send an on-chain payment with the given fee.
+    pub async fn send(
+        &self,
+        address: &Address<NetworkUnchecked>,


Conceptually we should be passing Address (which is the same as Address<NetworkChecked> around. We should only need to deal with NetworkUnchecked on the boundaries of our system, check them fast and not have to worry about it anymore. Ping @tvolk131 if that's our approach, or are there any things preventing it.

Hmm... after noticing the check just below. I guess it makes sense to validate it here, since this is a public method that a downstream might use. Though... maybe we should pass that responsibilty to the user (that's what using Address<NetworkChecked> would do). Possibly they already validated it anyway, and they already have it checked.

We could have a public method fn validate_address_network(&self, address: Address<UncheckedNetwork>) -> Result<Address<CheckedNetwork>>)

I think it makes sense to validate it here. It'd be confusing to API consumers for us to perform other checks here such as ensuring we're above the dust amount and that the fee rate is high enough, but then perform the network validation in a completely different way. If we leave it up to the user, they could accidentally bypass this check by calling assume_checked(). If an API consumer has an address that they already checked, they can always call Address::into_unchecked() to pass it in here.

@joschisan if we do decide to keep validating the network here, can you call Address::require_network() instead of is_valid_for_network() to idiomatically turn the unchecked network into a checked network, so that we no longer need to call assume_checked() below?

dpc · 2024-12-17T20:48:07Z

modules/fedimint-walletv2-client/src/lib.rs

+        Some(SafeUrl::parse(url).expect("Failed to parse default esplora server"))
+    }
+
+    /// Check an address for unspent deposits and return the deposits in


The format of a docstring comment should be a "single sentence, newlines, longer description". I thought clippy nags if it isn't the case.

dpc · 2024-12-17T20:50:20Z

modules/fedimint-walletv2-client/src/lib.rs

+    /// Fetch the current fee required to issue ecash for an unspent deposit.
+    pub async fn receive_fee(&self) -> Result<bitcoin::Amount, ReceiveError> {
+        self.module_api
+            .receive_fee()


Do these often change? Would it make sense to cache the result or just fetch it on init? A caller user might not be aware this is actually making API call and spam it unware. If we can't/don't want to make it cached, we should name it fetch_receive_fee or something at least.

dpc · 2024-12-17T20:50:55Z

modules/fedimint-walletv2-client/src/lib.rs

+            None => receive_fee,
+        };
+
+        if unspent_deposit.value < receive_fee.value + bitcoin::Amount::from_sat(100) {


Magic constant, maybe make it a constant with a docstring.

dpc · 2024-12-17T20:52:36Z

modules/fedimint-walletv2-common/src/config.rs

+                    + change_input_weight
+                    + change_output_weight,
+            ),
+            dust_limit: bitcoin::Amount::from_sat(10_000),


Different magic constant.

dpc · 2024-12-17T20:53:23Z

modules/fedimint-walletv2-common/src/config.rs

+
+        Self {
+            bitcoin_pks,
+            finality_delay: 6,


Magic constant, also load bearing I guess.

dpc · 2024-12-17T20:59:59Z

modules/fedimint-walletv2-common/src/config.rs

+        write!(
+            f,
+            "WalletClientConfig {}",
+            serde_json::to_string(self).map_err(|_e| std::fmt::Error)?


Do we really need Display on this one? The implementation here looks like of like Debug anyway, not very usable for ... "output to the end user", if you ask me, and probably the whole struct shouldn't need it really in the first place. {:?} in formatting will produce roughly same thing without serde_json.

dpc · 2024-12-17T21:01:32Z

modules/fedimint-walletv2-common/src/esplora_api.rs

+use serde::{Deserialize, Serialize};
+
+#[apply(async_trait_maybe_send!)]
+pub trait IEsploraConnection: Debug {


Though: maybe we should move it to fedimint-bitcoind src/esplora.rs. I mean ... essentially it is a bitcoin network backend, just slightly different interface.

dpc · 2024-12-17T21:02:42Z

modules/fedimint-walletv2-common/src/lib.rs

+            }
+        }
+
+        panic!("Failed to obtain Destination from address");


Maybe better as a Result? This panic in a public function could sneak on someone downstream.

dpc · 2024-12-17T21:07:02Z

modules/fedimint-walletv2-common/src/lib.rs

+#[derive(Debug, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, Encodable, Decodable)]
+pub struct WalletOutputOutcomeV0(pub bitcoin::Txid);
+
+impl std::fmt::Display for WalletOutputOutcomeV0 {


Here and in other places, these Display impls don't sit well with me. Display should be impled when something has a very clear, natural and standard conversion to string that people can depend on. Changing Display impl shouldn't be treated lightly as someone might depend on it.

Here, it seems to me that using/customizing impl fmt::Debug is the better approach.

Sometimes it is also worthwhile to make a custom formatting newtype if a given formatting is used for a very specific purpose, and standard fmt::Debug doesn't work well for it (e.g. struct WalletOutputOutcomeV0FmtForConsensusDebug(&WalletOutputOutcomeV0))

dpc · 2024-12-17T21:10:21Z

modules/fedimint-walletv2-server/src/db.rs

@@ -0,0 +1,177 @@
+use bitcoin::{TxOut, Txid};


Note to self: restart reviewing from this spot. :D

tvolk131 · 2024-12-19T14:54:34Z

modules/fedimint-walletv2-client/src/db.rs

+use serde::Serialize;
+use strum_macros::EnumIter;
+
+// #[repr(u8)]


Nit: Remove

tvolk131 · 2024-12-19T15:21:17Z

modules/fedimint-walletv2-client/src/lib.rs

+    /// Send an on-chain payment with the given fee.
+    pub async fn send(
+        &self,
+        address: &Address<NetworkUnchecked>,


I think it makes sense to validate it here. It'd be confusing to API consumers for us to perform other checks here such as ensuring we're above the dust amount and that the fee rate is high enough, but then perform the network validation in a completely different way. If we leave it up to the user, they could accidentally bypass this check by calling assume_checked(). If an API consumer has an address that they already checked, they can always call Address::into_unchecked() to pass it in here.

@joschisan if we do decide to keep validating the network here, can you call Address::require_network() instead of is_valid_for_network() to idiomatically turn the unchecked network into a checked network, so that we no longer need to call assume_checked() below?

tvolk131 · 2024-12-19T15:48:55Z

modules/fedimint-walletv2-server/src/lib.rs

+        // If we cannot fetch a up to date feerate from our bitcoin backend we need to
+        // retract our last vote on the feerate as an outdated consensus feerate can get
+        // our transactions stuck.
+        if let Ok(Some(fee_rate)) = self.btc_rpc.get_fee_rate(CONFIRMATION_TARGET).await {
+            items.push(WalletConsensusItem::Feerate(Some(fee_rate.sats_per_kvb)));
+        } else {
+            // Regtest node never returns fee rate
+            if self.cfg.consensus.network == Network::Regtest {
+                items.push(WalletConsensusItem::Feerate(Some(1000)));
+            } else {
+                items.push(WalletConsensusItem::Feerate(None));
+            }
+        }


What about this?

// Update our fee rate vote let fee_rate_vote = if let Ok(Some(fee_rate)) = self.btc_rpc.get_fee_rate(CONFIRMATION_TARGET).await { Some(fee_rate.sats_per_kvb) } else if self.cfg.consensus.network == Network::Regtest { // Regtest node never returns fee rate Some(1000) } else { // If we cannot fetch an up to date feerate from our bitcoin backend we need to // retract our last vote on the feerate as an outdated consensus feerate can get // our transactions stuck. None } items.push(items.push(WalletConsensusItem::Feerate(fee_rate_vote)));

tvolk131 · 2024-12-19T16:33:44Z

modules/fedimint-walletv2-server/src/lib.rs

+                // We do not sync blocks that predate the federation itself.
+                if old_consensus_block_count == 0 {
+                    return Ok(());
+                }


I'm trying to wrap my head around why this is safe. I don't see anything preventing an on-chain receive from occurring before a consensus block is reached. And I see that in some of the tests you put this comment:

// We need the consensus block count to reach a non-zero value before we send in // any funds such that the UTXO is tracked by the federation.

Are we assuming that the guardians will reach block consensus before inviting anyone to their federation? That seems like a reasonable assumption to make, but we should make that requirement clear.

tvolk131 · 2024-12-19T16:45:07Z

modules/fedimint-walletv2-tests/bin/tests.rs

+        if fedimint_cli_version < *VERSION_0_5_0_ALPHA {
+            info!(%fedimint_cli_version, "Version did not support walletv2 module, skipping");
+            return Ok(());
+        }
+
+        if fedimintd_version < *VERSION_0_5_0_ALPHA {
+            info!(%fedimintd_version, "Version did not support walletv2 module, skipping");
+            return Ok(());
+        }


Nit: Can we combine these?

if fedimint_cli_version < *VERSION_0_5_0_ALPHA || fedimintd_version < *VERSION_0_5_0_ALPHA { info!(%fedimint_cli_version, "Version did not support walletv2 module, skipping"); return Ok(()); }

joschisan requested review from a team as code owners November 21, 2024 06:53

joschisan force-pushed the walletv2 branch 2 times, most recently from bf6127b to 68aae67 Compare November 21, 2024 08:44

joschisan requested a review from a team as a code owner November 21, 2024 08:44

joschisan force-pushed the walletv2 branch 25 times, most recently from 41ee984 to 2e36e7a Compare November 23, 2024 16:15

dpc reviewed Dec 17, 2024

View reviewed changes

tvolk131 requested changes Dec 19, 2024

View reviewed changes

joschisan force-pushed the walletv2 branch from df571a0 to f429d63 Compare December 27, 2024 11:16

joschisan marked this pull request as draft January 18, 2025 14:45

feat: initial implementation of walletv2

f922c6e

joschisan force-pushed the walletv2 branch from f429d63 to f922c6e Compare January 18, 2025 14:52

feat: transaction log

8b3caa4

joschisan force-pushed the walletv2 branch from 2a06416 to 8b3caa4 Compare January 31, 2025 06:12

cleanup: remove output outcome

83b5015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial implementation of walletv2 #6429

feat: initial implementation of walletv2 #6429

joschisan commented Nov 21, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

tvolk131 Dec 19, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024

dpc Dec 17, 2024

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024

dpc Dec 17, 2024

tvolk131 Dec 19, 2024

tvolk131 Dec 19, 2024

tvolk131 Dec 19, 2024

tvolk131 Dec 19, 2024

tvolk131 Dec 19, 2024

feat: initial implementation of walletv2 #6429

Are you sure you want to change the base?

feat: initial implementation of walletv2 #6429

Conversation

joschisan commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading

dpc Dec 17, 2024 •

edited

Loading