More tests, inputs ordering (iotaledger#1585)

* Remove bech32_address field from InputSigningData

* More tests, inputs ordering

* Rebase fixes, bindings

* Bring back extend and sort

* Update client/src/api/block_builder/input_selection/core/requirement/mod.rs

* Optional time

* Remove duplicated check

* Address review comments

* Update comment

* Use SecretManager::try_from_mnemonic

* Add unix_timestamp_now() and TODO comment

* Resolve conflict

---------

Co-authored-by: Thibault Martinez <[email protected]>

.changes/bech32-address.md

@@ -0,0 +1,6 @@
+
+---
+"nodejs-binding": patch
+---
+
+Removed `IInputSigningData::bech32Address`;

client/CHANGELOG.md

@@ -38,6 +38,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Removed
 
 - `ClientBlockBuilder::get_output_amount_and_address()`;
+- `{InputSigningData, InputSigningDataDto}::bech32_address`;
+- Added time parameter to `SecretManageExt::sign_transaction_essence()` and `SecretManager::default_sign_transaction_essence()`;
 
 ### Fixed
 

client/bindings/nodejs/types/preparedTransactionData.ts

@@ -42,11 +42,6 @@ export interface IInputSigningData {
      * The chain derived from seed, only for ed25519 addresses
      */
     chain?: ISegment[];
-    /**
-     * The bech32 encoded address, required because of alias outputs where we have multiple possible unlock
-     * conditions, because we otherwise don't know which one we need
-     */
-    bech32Address: string;
 }
 
 export interface IRemainder {

client/bindings/wasm/CHANGELOG.md

@@ -26,6 +26,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Changes from the Rust library;
 - Merged `IAuth::{username, password}` into `IAuth::basicAuthNamePwd`;
 
+### Removed
+
+- `IInputSigningData::bech32Address`;
+
 ## 1.0.0-alpha.2 - 2023-02-08
 
 ### Added

client/examples/offline_signing/2_transaction_signing.rs

@@ -32,7 +32,7 @@ async fn main() -> Result<()> {
 
     // Signs the prepared transaction offline.
     let unlocks = secret_manager
-        .sign_transaction_essence(&prepared_transaction_data)
+        .sign_transaction_essence(&prepared_transaction_data, None)
         .await?;
     let signed_transaction = TransactionPayload::new(prepared_transaction_data.essence.clone(), unlocks)?;
 

client/src/api/block_builder/input_selection/automatic.rs

@@ -18,12 +18,13 @@ use iota_types::{
 use crate::{
     api::{
         block_builder::input_selection::core::{Error as InputSelectionError, InputSelection, Selected},
+        input_selection::is_alias_transition,
         ClientBlockBuilder, ADDRESS_GAP_RANGE,
     },
     constants::HD_WALLET_TYPE,
     node_api::indexer::query_parameters::QueryParameter,
     secret::types::InputSigningData,
-    Error, Result,
+    unix_timestamp_now, Error, Result,
 };
 
 impl<'a> ClientBlockBuilder<'a> {
@@ -49,12 +50,7 @@ impl<'a> ClientBlockBuilder<'a> {
                     QueryParameter::HasExpiration(true),
                     QueryParameter::HasStorageDepositReturn(false),
                     // Ignore outputs that aren't expired yet
-                    QueryParameter::ExpiresBefore(
-                        instant::SystemTime::now()
-                            .duration_since(instant::SystemTime::UNIX_EPOCH)
-                            .expect("time went backwards")
-                            .as_secs() as u32,
-                    ),
+                    QueryParameter::ExpiresBefore(unix_timestamp_now()),
                 ])
                 .await?,
         );
@@ -77,6 +73,7 @@ impl<'a> ClientBlockBuilder<'a> {
 
         // First get inputs for utxo chains (Alias, Foundry, NFT outputs).
         let mut available_inputs = self.get_utxo_chains_inputs(self.outputs.iter()).await?;
+
         let required_inputs_for_sender_or_issuer = self.get_inputs_for_sender_and_issuer(&available_inputs).await?;
         let required_inputs_for_sender_or_issuer_ids = required_inputs_for_sender_or_issuer
             .iter()
@@ -87,13 +84,21 @@ impl<'a> ClientBlockBuilder<'a> {
         available_inputs.sort_unstable_by_key(|input| *input.output_id());
         available_inputs.dedup_by_key(|input| *input.output_id());
 
-        // Assume that we own the addresses for inputs that are required for the provided outputs
-        let mut available_input_addresses = available_inputs
-            .iter()
-            .map(|input| Ok(Address::try_from_bech32(&input.bech32_address)?.1))
-            .collect::<Result<Vec<Address>>>()?;
-
         let current_time = self.client.get_time_checked().await?;
+        // Assume that we own the addresses for inputs that are required for the provided outputs
+        let mut available_input_addresses = Vec::new();
+        for input in &available_inputs {
+            let alias_transition = is_alias_transition(input, &self.outputs);
+            let (required_unlock_address, unlocked_alias_or_nft_address) = input.output.required_and_unlocked_address(
+                current_time,
+                input.output_id(),
+                alias_transition.map(|(alias_transition, _)| alias_transition),
+            )?;
+            available_input_addresses.push(required_unlock_address);
+            if let Some(unlocked_alias_or_nft_address) = unlocked_alias_or_nft_address {
+                available_input_addresses.push(unlocked_alias_or_nft_address);
+            }
+        }
 
         // Try to select inputs with required inputs for utxo chains alone before requesting more inputs from addresses.
         let mut input_selection = InputSelection::new(
@@ -193,7 +198,6 @@ impl<'a> ClientBlockBuilder<'a> {
                                     *internal as u32,
                                     address_index,
                                 ])),
-                                bech32_address: str_address.clone(),
                             });
                         }
                     }

client/src/api/block_builder/input_selection/core/mod.rs

@@ -9,7 +9,9 @@ pub(crate) mod transition;
 
 use std::collections::{HashMap, HashSet};
 
-use self::requirement::alias::is_alias_transition;
+use packable::PackableExt;
+pub(crate) use requirement::is_alias_transition;
+
 pub use self::{
     burn::{Burn, BurnDto},
     error::Error,
@@ -18,11 +20,12 @@ pub use self::{
 use crate::{
     api::types::RemainderData,
     block::{
-        address::{Address, AliasAddress, Ed25519Address, NftAddress},
+        address::{Address, AliasAddress, NftAddress},
         output::{AliasTransition, ChainId, Output, OutputId},
         protocol::ProtocolParameters,
     },
     secret::types::InputSigningData,
+    unix_timestamp_now,
 };
 
 // TODO make methods actually take self? There was a mut issue.
@@ -184,10 +187,7 @@ impl InputSelection {
             burn: None,
             remainder_address: None,
             protocol_parameters,
-            timestamp: instant::SystemTime::now()
-                .duration_since(instant::SystemTime::UNIX_EPOCH)
-                .expect("time went backwards")
-                .as_secs() as u32,
+            timestamp: unix_timestamp_now(),
             requirements: Vec::new(),
             automatically_transitioned: HashMap::new(),
         }
@@ -254,35 +254,56 @@ impl InputSelection {
     }
 
     // Inputs need to be sorted before signing, because the reference unlock conditions can only reference a lower index
-    pub(crate) fn sort_input_signing_data(inputs: Vec<InputSigningData>) -> Result<Vec<InputSigningData>, Error> {
-        // filter for ed25519 address first, safe to unwrap since we encoded it before
-        let (mut sorted_inputs, alias_nft_address_inputs): (Vec<InputSigningData>, Vec<InputSigningData>) = inputs
-            .into_iter()
-            // PANIC: safe to unwrap as we encoded the address before
-            .partition(|input| {
-                Address::try_from_bech32(&input.bech32_address).unwrap().1.kind() == Ed25519Address::KIND
+    pub(crate) fn sort_input_signing_data(
+        mut inputs: Vec<InputSigningData>,
+        outputs: &[Output],
+        time: Option<u32>,
+    ) -> Result<Vec<InputSigningData>, Error> {
+        let time = time.unwrap_or_else(unix_timestamp_now);
+        // initially sort by output to make it deterministic
+        // TODO: rethink this, we only need it deterministic for tests, for the protocol it doesn't matter, also there
+        // might be a more efficient way to do this
+        inputs.sort_by_key(|i| i.output.pack_to_vec());
+        // filter for ed25519 address first
+        let (mut sorted_inputs, alias_nft_address_inputs): (Vec<InputSigningData>, Vec<InputSigningData>) =
+            inputs.into_iter().partition(|input_signing_data| {
+                let alias_transition = is_alias_transition(input_signing_data, outputs);
+                let (input_address, _) = input_signing_data
+                    .output
+                    .required_and_unlocked_address(
+                        time,
+                        input_signing_data.output_id(),
+                        alias_transition.map(|(alias_transition, _)| alias_transition),
+                    )
+                    // PANIC: safe to unwrap, because we filtered treasury outputs out before
+                    .unwrap();
+
+                input_address.is_ed25519()
             });
 
         for input in alias_nft_address_inputs {
-            let input_address = Address::try_from_bech32(&input.bech32_address);
+            let alias_transition = is_alias_transition(&input, outputs);
+            let (input_address, _) = input.output.required_and_unlocked_address(
+                time,
+                input.output_id(),
+                alias_transition.map(|(alias_transition, _)| alias_transition),
+            )?;
+
             match sorted_inputs.iter().position(|input_signing_data| match input_address {
-                Ok((_, unlock_address)) => match unlock_address {
-                    Address::Alias(unlock_address) => {
-                        if let Output::Alias(alias_output) = &input_signing_data.output {
-                            *unlock_address.alias_id() == alias_output.alias_id_non_null(input_signing_data.output_id())
-                        } else {
-                            false
-                        }
+                Address::Alias(unlock_address) => {
+                    if let Output::Alias(alias_output) = &input_signing_data.output {
+                        *unlock_address.alias_id() == alias_output.alias_id_non_null(input_signing_data.output_id())
+                    } else {
+                        false
                     }
-                    Address::Nft(unlock_address) => {
-                        if let Output::Nft(nft_output) = &input_signing_data.output {
-                            *unlock_address.nft_id() == nft_output.nft_id_non_null(input_signing_data.output_id())
-                        } else {
-                            false
-                        }
+                }
+                Address::Nft(unlock_address) => {
+                    if let Output::Nft(nft_output) = &input_signing_data.output {
+                        *unlock_address.nft_id() == nft_output.nft_id_non_null(input_signing_data.output_id())
+                    } else {
+                        false
                     }
-                    _ => false,
-                },
+                }
                 _ => false,
             }) {
                 Some(position) => {
@@ -304,10 +325,18 @@ impl InputSelection {
                     if let Some(alias_or_nft_address) = alias_or_nft_address {
                         // Check for existing outputs for this address, and insert before
                         match sorted_inputs.iter().position(|input_signing_data| {
-                            Address::try_from_bech32(&input_signing_data.bech32_address)
-                                .expect("safe to unwrap, we encoded it before")
-                                .1
-                                == alias_or_nft_address
+                            let alias_transition = is_alias_transition(input_signing_data, outputs);
+                            let (input_address, _) = input_signing_data
+                                .output
+                                .required_and_unlocked_address(
+                                    time,
+                                    input.output_id(),
+                                    alias_transition.map(|(alias_transition, _)| alias_transition),
+                                )
+                                // PANIC: safe to unwrap, because we filtered treasury outputs out before
+                                .unwrap();
+
+                            input_address == alias_or_nft_address
                         }) {
                             Some(position) => {
                                 // Insert before the output with this address required for unlocking
@@ -362,7 +391,7 @@ impl InputSelection {
         self.outputs.extend(storage_deposit_returns);
 
         Ok(Selected {
-            inputs: Self::sort_input_signing_data(self.selected_inputs)?,
+            inputs: Self::sort_input_signing_data(self.selected_inputs, &self.outputs, Some(self.timestamp))?,
             outputs: self.outputs,
             remainder,
         })

client/src/api/block_builder/input_selection/core/requirement/mod.rs

@@ -10,6 +10,7 @@ pub(crate) mod native_tokens;
 pub(crate) mod nft;
 pub(crate) mod sender;
 
+pub(crate) use self::alias::is_alias_transition;
 use self::{alias::is_alias_with_id_non_null, foundry::is_foundry_with_id, nft::is_nft_with_id_non_null};
 use super::{Error, InputSelection};
 use crate::{

client/src/api/block_builder/input_selection/manual.rs

@@ -16,7 +16,7 @@ use crate::{
     api::{
         address::search_address,
         block_builder::input_selection::{Burn, InputSelection, Selected},
-        input_selection::core::requirement::alias::is_alias_transition_internal,
+        input_selection::{core::requirement::alias::is_alias_transition_internal, is_alias_transition},
         ClientBlockBuilder,
     },
     constants::HD_WALLET_TYPE,
@@ -88,7 +88,6 @@ impl<'a> ClientBlockBuilder<'a> {
                                 address_index,
                             ])
                         }),
-                        bech32_address: unlock_address.to_bech32(&bech32_hrp),
                     });
                 }
             }
@@ -100,10 +99,19 @@ impl<'a> ClientBlockBuilder<'a> {
             .collect::<HashSet<_>>();
 
         // Assume that we own the addresses for inputs that are provided
-        let available_input_addresses = inputs_data
-            .iter()
-            .map(|input| Ok(Address::try_from_bech32(&input.bech32_address)?.1))
-            .collect::<Result<Vec<Address>>>()?;
+        let mut available_input_addresses = Vec::new();
+        for input in &inputs_data {
+            let alias_transition = is_alias_transition(input, &self.outputs);
+            let (required_unlock_address, unlocked_alias_or_nft_address) = input.output.required_and_unlocked_address(
+                current_time,
+                input.output_id(),
+                alias_transition.map(|(alias_transition, _)| alias_transition),
+            )?;
+            available_input_addresses.push(required_unlock_address);
+            if let Some(unlocked_alias_or_nft_address) = unlocked_alias_or_nft_address {
+                available_input_addresses.push(unlocked_alias_or_nft_address);
+            }
+        }
 
         inputs_data.sort_unstable_by_key(|input| *input.output_id());
         inputs_data.dedup_by_key(|input| *input.output_id());

client/src/api/block_builder/input_selection/mod.rs

@@ -10,6 +10,7 @@ mod manual;
 mod sender_issuer;
 mod utxo_chains;
 
+pub(crate) use self::core::is_alias_transition;
 pub use self::{
     core::{Burn, BurnDto, Error, InputSelection, Requirement, Selected},
     helpers::minimum_storage_deposit_basic_output,

client/src/api/block_builder/input_selection/sender_issuer.rs

@@ -79,7 +79,6 @@ impl<'a> ClientBlockBuilder<'a> {
                                     internal as u32,
                                     address_index,
                                 ])),
-                                bech32_address: sender_or_issuer_address.to_bech32(&bech32_hrp),
                             });
                             found_output = true;
                             break;
@@ -142,7 +141,6 @@ impl<'a> ClientBlockBuilder<'a> {
                                         address_index,
                                     ])
                                 }),
-                                bech32_address: unlock_address.to_bech32(&bech32_hrp),
                             });
                         }
                     }
@@ -202,7 +200,6 @@ impl<'a> ClientBlockBuilder<'a> {
                                         address_index,
                                     ])
                                 }),
-                                bech32_address: unlock_address.to_bech32(&bech32_hrp),
                             });
                         }
                     }

client/src/api/block_builder/input_selection/utxo_chains.rs

@@ -212,7 +212,6 @@ impl<'a> ClientBlockBuilder<'a> {
                         address_index,
                     ])
                 }),
-                bech32_address: unlock_address.to_bech32(&bech32_hrp),
             });
         }
 

client/src/api/block_builder/transaction.rs

@@ -89,15 +89,15 @@ impl<'a> ClientBlockBuilder<'a> {
     pub async fn sign_transaction(&self, prepared_transaction_data: PreparedTransactionData) -> Result<Payload> {
         log::debug!("[sign_transaction] {:?}", prepared_transaction_data);
         let secret_manager = self.secret_manager.ok_or(Error::MissingParameter("secret manager"))?;
+        let current_time = self.client.get_time_checked().await?;
+
         let unlocks = secret_manager
-            .sign_transaction_essence(&prepared_transaction_data)
+            .sign_transaction_essence(&prepared_transaction_data, Some(current_time))
             .await?;
         let tx_payload = TransactionPayload::new(prepared_transaction_data.essence.clone(), unlocks)?;
 
         validate_transaction_payload_length(&tx_payload)?;
 
-        let current_time = self.client.get_time_checked().await?;
-
         let conflict = verify_semantic(&prepared_transaction_data.inputs_data, &tx_payload, current_time)?;
 
         if conflict != ConflictReason::None {

client/src/api/high_level.rs

@@ -25,7 +25,7 @@ use crate::{
     error::{Error, Result},
     node_api::indexer::query_parameters::QueryParameter,
     secret::SecretManager,
-    Client,
+    unix_timestamp_now, Client,
 };
 
 impl Client {
@@ -334,10 +334,7 @@ impl Client {
     /// Returns the local time checked with the timestamp of the latest milestone, if the difference is larger than 5
     /// minutes an error is returned to prevent locking outputs by accident for a wrong time.
     pub async fn get_time_checked(&self) -> Result<u32> {
-        let current_time = instant::SystemTime::now()
-            .duration_since(instant::SystemTime::UNIX_EPOCH)
-            .expect("time went backwards")
-            .as_secs() as u32;
+        let current_time = unix_timestamp_now();
 
         let network_info = self.get_network_info().await?;