add additional UTXO info to partially signed tx #1799

OBorce · 2024-08-06T22:39:52Z

add additional UTXO info for tokens (number of decimals and ticker) and pools (balance) to partially signed tx, intended to be shown on a hardware wallet UI like Trezor.

chainstate/src/rpc/types/output.rs

common/src/chain/tokens/rpc.rs

chainstate/src/rpc/mod.rs

ImplOfAnImpl · 2024-08-07T18:16:53Z

common/src/chain/transaction/partially_signed_transaction.rs

+/// Additional info for UTXOs
+#[derive(Debug, Eq, PartialEq, Clone, Encode, Decode)]
+pub enum UtxoAdditionalInfo {
+    TokenInfo { num_decimals: u8, ticker: Vec<u8> },
+    PoolInfo { staker_balance: Amount },
+    NoAdditionalInfo,
+}
+
 #[derive(Debug, Eq, PartialEq, Clone, Encode, Decode)]
 pub struct PartiallySignedTransaction {
    tx: Transaction,
    witnesses: Vec<Option<InputWitness>>,

-    input_utxos: Vec<Option<TxOutput>>,
+    input_utxos: Vec<Option<(TxOutput, UtxoAdditionalInfo)>>,
    destinations: Vec<Option<Destination>>,
 }

 impl PartiallySignedTransaction {
    pub fn new(
        tx: Transaction,
        witnesses: Vec<Option<InputWitness>>,
-        input_utxos: Vec<Option<TxOutput>>,
+        input_utxos: Vec<Option<(TxOutput, UtxoAdditionalInfo)>>,
        destinations: Vec<Option<Destination>>,
    ) -> Result<Self, TransactionCreationError> {


This is indeed rather hacky. Can we put this info somewhere beside PartiallySignedTransaction instead?

I agree that the way it's done right now is hacky.
Maybe moving PartiallySignedTransaction from wallet to common was not that good idea after all. I did that few months ago because the struct looked really basic and a good companion for SignedTransaction, also I thought I'd use it in tx-verifier. But maybe I misunderstood it and the type is from wallet layer. Thoughts?

wallet/wallet-node-client/Cargo.toml

ImplOfAnImpl · 2024-08-19T08:42:22Z

common/src/chain/tokens/tokens_utils.rs

+        | TxOutput::IssueFungibleToken(_)
+        | TxOutput::AnyoneCanTake(_) => None,
+        TxOutput::IssueNft(_, _, _) => make_token_id(inputs),


Why is IssueFungibleToken ignored here?

Btw, this changes the logic of the wallet's OutputCache::find_unspent_unlocked_utxo/utxos_with_token_ids, which previously were producing token ids both for ft and nft. I wonder if it's intentional.

IssueFungibleToken is ignored as it can't be spent as a utxo, this was added to fix the utxo functions, because they were not updated when TokenV1 was added, so they were only handeling the TokensV0 and NFT.

Returning token id only for spendable output is a very specific logic, common crate might not be the best place for it. Why not moving it to the wallet and renaming to get_token_id_for_spendable_utxo?

removed the token ids being returned for the utxos, and added a separate call to fetch them at the places where they are needed to be checked for frozen tokens.

common/src/chain/transaction/partially_signed_transaction.rs

wallet/src/account/output_cache/mod.rs

ImplOfAnImpl · 2024-08-19T09:57:42Z

wallet/src/send_request/mod.rs

+                        TxOutput::AnyoneCanTake(data) => {
+                            find_additional_info(data.as_ref().ask(), additional_info)?


I might be missing something, but does it make sense to handle AnyoneCanTake here? I mean, is it a spendable output? I had the impression that it is not, @azarovh could you clarify?

~~It is spendable by anyone who will provide "asked" currency in exchange.~~

P.S. I think data.as_ref().ask() is wrong and it should be give() which is the value that creator of the output locked with it.

~~Also this value can be a token so I think it does make sense~~

Sorry guys I confused everyone here. AnyoneCanTake output is not included into utxo set (but rather creates an account) and is not spendable.

Sorry guys I confused everyone here. AnyoneCanTake output is not included into utxo set (but rather creates an account) and is not spendable.

Can you add some doc comment for AnyoneCanTake describing its nuances?

this is still present as we not only need additional info for UTXOs but also for the output UTXOs

ImplOfAnImpl · 2024-08-19T10:05:16Z

wallet/src/send_request/mod.rs

+    pub fn into_partially_signed_tx(
+        self,
+        additional_info: &BTreeMap<PoolOrTokenId, UtxoAdditionalInfo>,
+    ) -> WalletResult<PartiallySignedTransaction> {


I wonder if we can put additional_info inside SendRequest, so that there is less chance for them to become out of sync.

The match statement in fetch_utxo_exra_info has the same logic. I wonder if we can factor it out. Or maybe put the two implementations near each other so that it's clear that they must be in sync.

azarovh

I have small additional request: is_v0_token_output should check that values inside OrderData of AnyoneCanTake output does not contain tokens v0.

azarovh · 2024-08-20T08:00:56Z

wallet/wallet-controller/src/helpers.rs

+    }
+}
+
+pub async fn fetch_utxo_exra_info<T>(


typo: extra

azarovh · 2024-08-22T15:54:44Z

wallet/types/src/partially_signed_transaction.rs

    witnesses: Vec<Option<InputWitness>>,

-    input_utxos: Vec<Option<TxOutput>>,
+    input_utxos: Vec<Option<UtxoWithAdditionalInfo>>,


why not storing utxo separately from info and zipping if necessary? Right now the data is duplicated. Also it looks cleaner with all the data inside ptx flatten. If such new type is required for trezor I'd move it closer to it.

I went with this to preserve the invariant that you can't have additional info if you don't have an UTXO as well. Also the none case of the optional for the additional info does not mean missing it means no additional info. Maybe I should go back to the enum instead of the option + enum?

I see. Would it work if output_additional_infos field is removed and the getter return mapped Iter over input_utxos?

Now I get it. I'd leave a comment about that invariant and new field just to make it clear for the future.

common/src/chain/tokens/tokens_utils.rs

azarovh · 2024-08-23T09:44:25Z

wallet/src/send_request/mod.rs

+        let output_additional_infos = self
+            .outputs
+            .iter()
+            .map(|utxo| find_additional_info(utxo, additional_info))


So yeah this should be None or even an error because it's not spendable and cannot be a utxo.

OBorce force-pushed the refactor/partially-signed-tx-info branch from 2c28c54 to ef80f00 Compare August 6, 2024 22:58

ImplOfAnImpl reviewed Aug 7, 2024

View reviewed changes

OBorce force-pushed the refactor/wallet-generic-signer branch from 1cd8110 to c23f38d Compare August 13, 2024 23:23

OBorce force-pushed the refactor/partially-signed-tx-info branch from dae3bf3 to 51a9d60 Compare August 15, 2024 13:56

OBorce marked this pull request as ready for review August 16, 2024 15:26

OBorce requested review from azarovh and erubboli as code owners August 16, 2024 15:26

azarovh reviewed Aug 19, 2024

View reviewed changes

wallet/wallet-node-client/Cargo.toml Outdated Show resolved Hide resolved

ImplOfAnImpl reviewed Aug 19, 2024

View reviewed changes

azarovh reviewed Aug 20, 2024

View reviewed changes

wallet/wallet-controller/src/helpers.rs Outdated

}

}

pub async fn fetch_utxo_exra_info<T>(

Copy link

Contributor

azarovh Aug 20, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

typo: extra

azarovh reviewed Aug 22, 2024

View reviewed changes

azarovh reviewed Aug 23, 2024

View reviewed changes

OBorce force-pushed the refactor/partially-signed-tx-info branch from 6444490 to b31700d Compare August 26, 2024 21:15

OBorce added 12 commits August 28, 2024 18:04

add additional UTXO info to partially signed tx

9f9851c

update RPC docs

b02a872

add additional token info for utxos in partially signed tx

4d87cc2

clean up from review comments

d4d9ae7

update rpc docs

ebeb017

Add additional utxo info for tx outputs

b9ed105

move PartiallySignedTx to wallet

e053fda

refactor review comments

98016a3

refactor wallet utxo token_id uses

5778ebe

add separate additional data for anyonecanstake

fd0cf1f

add support for signing HTLC outputs with trezor

08c24ec

fix comments

43a08da

OBorce force-pushed the refactor/partially-signed-tx-info branch from 190d6f3 to 8d105c2 Compare August 30, 2024 14:58

extract runtime wallet functionality

c24ff7b

OBorce force-pushed the refactor/partially-signed-tx-info branch from 8d105c2 to c24ff7b Compare August 30, 2024 15:27

OBorce force-pushed the refactor/wallet-generic-signer branch from c23f38d to 4824da8 Compare August 30, 2024 16:09

fix functional tests

f7f72f3

OBorce merged commit 6e324c7 into refactor/wallet-generic-signer Sep 2, 2024

OBorce deleted the refactor/partially-signed-tx-info branch September 2, 2024 08:47

		TxOutput::AnyoneCanTake(data) => {
		find_additional_info(data.as_ref().ask(), additional_info)?

add additional UTXO info to partially signed tx #1799

add additional UTXO info to partially signed tx #1799

Uh oh!

Conversation

OBorce commented Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azarovh Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azarovh Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azarovh left a comment

Choose a reason for hiding this comment

Uh oh!

azarovh Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

azarovh Aug 19, 2024 •

edited

Loading

azarovh Aug 19, 2024 •

edited

Loading

azarovh Aug 20, 2024 •

edited

Loading