Merge pull request #1272 from radixdlt/bugfix/preview-free-credit

Fix transaction preview issues

.github/workflows/ci.yml

@@ -30,7 +30,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Check format
       run: bash ./check.sh
   sbor-unit-tests:
@@ -43,7 +43,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Run tests
       run: cargo test
       working-directory: sbor
@@ -63,7 +63,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Run tests
       run: cargo test
       working-directory: sbor-tests
@@ -83,7 +83,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Run tests
       run: cargo test
       working-directory: scrypto
@@ -106,7 +106,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Run tests
       run: cargo test
       working-directory: scrypto-tests
@@ -123,7 +123,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Add wasm target
       run: rustup target add wasm32-unknown-unknown
     - name: Add wasm target (nightly)
@@ -153,7 +153,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Install nextest
       uses: taiki-e/install-action@nextest
     - name: Install RocksDB metrics dependency
@@ -185,7 +185,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Install nextest
       uses: taiki-e/install-action@nextest
     - name: Add wasm target
@@ -206,7 +206,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Install nextest
       uses: taiki-e/install-action@nextest
     - name: Add wasm target
@@ -227,7 +227,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Add wasm target
       run: rustup target add wasm32-unknown-unknown
     - name: Run bench
@@ -243,7 +243,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Add wasm target
       run: rustup target add wasm32-unknown-unknown
     - name: Run bench
@@ -259,7 +259,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Run tests
       run: cargo test
       working-directory: transaction
@@ -287,7 +287,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - uses: radixdlt/rust-cache@allow_registry_src_caching
       with:
         prefix-key: ""
@@ -321,7 +321,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - uses: radixdlt/rust-cache@allow_registry_src_caching
       with:
         prefix-key: ""
@@ -349,7 +349,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-        toolchain: stable
+        toolchain: 1.70.0
     - name: Install nextest
       uses: taiki-e/install-action@nextest
     - name: Add wasm target

assets/template/src/lib.rs

@@ -14,10 +14,11 @@ mod hello {
         pub fn instantiate_hello() -> Global<Hello> {
             // Create a new token called "HelloToken," with a fixed supply of 1000, and put that supply into a bucket
             let my_bucket: Bucket = ResourceBuilder::new_fungible(OwnerRole::None)
+                .divisibility(DIVISIBILITY_MAXIMUM)
                 .metadata(metadata! {
                     init {
-                        "name" => "HelloToken".to_owned(), locked;
-                        "symbol" => "HT".to_owned(), locked;
+                        "name" => "HelloToken", locked;
+                        "symbol" => "HT", locked;
                     }
                 })
                 .mint_initial_supply(1000);

assets/template/tests/lib.rs

@@ -32,7 +32,11 @@ fn test_hello() {
     // Test the `free_token` method.
     let manifest = ManifestBuilder::new()
         .call_method(component, "free_token", manifest_args!())
-        .deposit_batch(account)
+        .call_method(
+            account,
+            "deposit_batch",
+            manifest_args!(ManifestExpression::EntireWorktop),
+        )
         .build();
     let receipt = test_runner.execute_manifest_ignoring_fee(
         manifest,

examples/hello-world/src/lib.rs

@@ -17,8 +17,8 @@ mod hello {
                 .divisibility(DIVISIBILITY_MAXIMUM)
                 .metadata(metadata! {
                     init {
-                        "name" => "HelloToken".to_owned(), locked;
-                        "symbol" => "HT".to_owned(), locked;
+                        "name" => "HelloToken", locked;
+                        "symbol" => "HT", locked;
                     }
                 })
                 .mint_initial_supply(1000);

radix-engine-tests/tests/preview.rs

@@ -1,3 +1,4 @@
+use radix_engine::system::system_modules::costing::FeeTable;
 use radix_engine::transaction::ExecutionConfig;
 use radix_engine::transaction::FeeReserveConfig;
 use radix_engine::types::*;
@@ -27,25 +28,62 @@ fn test_transaction_preview_cost_estimate() {
         manifest,
         &preview_flags,
     );
+    let size_diff = manifest_encode(&notarized_transaction).unwrap().len()
+        - manifest_encode(&preview_intent.intent).unwrap().len();
 
     // Act & Assert: Execute the preview, followed by a normal execution.
     // Ensure that both succeed and that the preview result provides an accurate cost estimate
-    let preview_result = test_runner.preview(preview_intent, &network);
-    let preview_receipt = preview_result.unwrap();
-    preview_receipt.expect_commit_success();
-
-    let receipt = test_runner.execute_transaction(
+    let preview_receipt = test_runner.preview(preview_intent, &network).unwrap();
+    let preview_result = preview_receipt.expect_commit_success();
+    let actual_receipt = test_runner.execute_transaction(
         validate(&network, &notarized_transaction).get_executable(),
         FeeReserveConfig::default(),
-        ExecutionConfig::for_preview(),
+        ExecutionConfig::for_notarized_transaction()
+            .with_kernel_trace(true)
+            .with_cost_breakdown(true),
     );
-    let commit_result = receipt.expect_commit(true);
+    let actual_result = actual_receipt.expect_commit_success();
     assert_eq!(
-        commit_result.fee_summary.execution_cost_sum,
-        commit_result.fee_summary.execution_cost_sum
+        // TODO: better preview payload size estimate?
+        preview_result.fee_summary.total_cost()
+            + FeeReserveConfig::default().cost_unit_price
+                * FeeTable::new().tx_payload_cost(size_diff),
+        actual_result.fee_summary.total_cost(),
     );
 }
 
+#[test]
+fn test_transaction_preview_without_locking_fee() {
+    // Arrange
+    let mut test_runner = TestRunner::builder().build();
+    let network = NetworkDefinition::simulator();
+    let manifest = ManifestBuilder::new()
+        // Explicitly don't lock fee from faucet
+        .clear_auth_zone()
+        .build();
+    let preview_flags = PreviewFlags {
+        use_free_credit: true,
+        assume_all_signature_proofs: false,
+        skip_epoch_check: false,
+    };
+    let (_, preview_intent) = prepare_matching_test_tx_and_preview_intent(
+        &mut test_runner,
+        &network,
+        manifest,
+        &preview_flags,
+    );
+
+    // Act
+    let preview_receipt = test_runner.preview(preview_intent, &network).unwrap();
+    let fee_summary = &preview_receipt.expect_commit_success().fee_summary;
+    println!("{:?}", preview_receipt);
+    assert_eq!(fee_summary.total_execution_cost_xrd, dec!("0.01669206"));
+    assert_eq!(fee_summary.total_tipping_cost_xrd, dec!("0"));
+    assert_eq!(fee_summary.total_state_expansion_cost_xrd, dec!("0.00009"));
+    assert_eq!(fee_summary.total_royalty_cost_xrd, dec!("0"));
+    assert_eq!(fee_summary.total_payments(), dec!("0")); // no one is paying the fees; wallets should fill the gap.
+}
+
 #[test]
 fn test_assume_all_signature_proofs_flag_method_authorization() {
     // Arrange

radix-engine/src/system/system_modules/costing/fee_summary.rs

@@ -53,6 +53,14 @@ impl FeeSummary {
             + self.total_royalty_cost_xrd
     }
 
+    pub fn total_payments(&self) -> Decimal {
+        self.fee_payments.values().cloned().sum::<Decimal>()
+    }
+
+    pub fn used_free_credit(&self) -> Decimal {
+        self.total_cost() - self.total_payments()
+    }
+
     //===================
     // For testing only
     //===================

radix-engine/src/transaction/transaction_executor.rs

@@ -124,6 +124,7 @@ impl ExecutionConfig {
     pub fn for_preview() -> Self {
         Self {
             enabled_modules: EnabledModules::for_preview(),
+            enable_cost_breakdown: true,
             ..Self::default()
         }
     }
@@ -137,6 +138,11 @@ impl ExecutionConfig {
         self
     }
 
+    pub fn with_cost_breakdown(mut self, enabled: bool) -> Self {
+        self.enable_cost_breakdown = enabled;
+        self
+    }
+
     pub fn with_cost_unit_limit(mut self, cost_unit_limit: u32) -> Self {
         self.cost_unit_limit = cost_unit_limit;
         self
@@ -174,31 +180,24 @@ where
 
     pub fn execute(
         &mut self,
-        transaction: &Executable,
+        executable: &Executable,
         fee_reserve_config: &FeeReserveConfig,
         execution_config: &ExecutionConfig,
     ) -> TransactionReceipt {
+        let free_credit = executable.fee_payment().free_credit_in_xrd;
+        let tip_percentage = executable.fee_payment().tip_percentage;
         let fee_reserve = SystemLoanFeeReserve::new(
             fee_reserve_config.cost_unit_price,
             fee_reserve_config.usd_price,
             fee_reserve_config.state_expansion_price,
-            transaction.fee_payment().tip_percentage,
+            tip_percentage,
             execution_config.cost_unit_limit,
             fee_reserve_config.system_loan,
             execution_config.abort_when_loan_repaid,
         )
-        .with_free_credit(transaction.fee_payment().free_credit_in_xrd);
+        .with_free_credit(free_credit);
+        let fee_table = FeeTable::new();
 
-        self.execute_with_fee_reserve(transaction, execution_config, fee_reserve, FeeTable::new())
-    }
-
-    fn execute_with_fee_reserve(
-        &mut self,
-        executable: &Executable,
-        execution_config: &ExecutionConfig,
-        fee_reserve: SystemLoanFeeReserve,
-        fee_table: FeeTable,
-    ) -> TransactionReceipt {
         // Dump executable
         #[cfg(not(feature = "alloc"))]
         if execution_config
@@ -279,8 +278,12 @@ where
                         }
 
                         // Distribute fees
-                        let (mut fee_summary, fee_payments) =
-                            Self::finalize_fees(&mut track, costing_module.fee_reserve, is_success);
+                        let (mut fee_summary, fee_payments) = Self::finalize_fees(
+                            &mut track,
+                            costing_module.fee_reserve,
+                            is_success,
+                            free_credit,
+                        );
                         fee_summary.execution_cost_breakdown = costing_module
                             .costing_traces
                             .into_iter()
@@ -604,6 +607,7 @@ where
         track: &mut Track<S, SpreadPrefixKeyMapper>,
         fee_reserve: SystemLoanFeeReserve,
         is_success: bool,
+        free_credit: Decimal,
     ) -> (FeeSummary, IndexMap<NodeId, Decimal>) {
         // Distribute royalty
         for (_, (recipient_vault_id, amount)) in fee_reserve.royalty_cost() {
@@ -667,16 +671,34 @@ where
             // Record final payments
             *fee_payments.entry(vault_id).or_default() += amount;
         }
+        // Free credit is locked first and thus used last
+        if free_credit.is_positive() {
+            let amount = Decimal::min(free_credit, required);
+            collected_fees.put(LiquidFungibleResource::new(amount));
+            required -= amount;
+        }
 
         let tips_to_distribute = fee_summary.tips_to_distribute();
         let fees_to_distribute = fee_summary.fees_to_distribute();
 
-        // Sanity check
-        assert_eq!(required, Decimal::ZERO);
-        assert_eq!(fee_summary.total_bad_debt_xrd, Decimal::ZERO);
-        assert_eq!(
-            tips_to_distribute + fees_to_distribute,
-            collected_fees.amount() - fee_summary.total_royalty_cost_xrd /* royalty already distributed */
+        // Sanity checks
+        assert!(
+            fee_summary.total_bad_debt_xrd == Decimal::ZERO,
+            "Bad debt is non-zero: {}",
+            fee_summary.total_bad_debt_xrd
+        );
+        assert!(
+            required == Decimal::ZERO,
+            "Locked fee does not cover transaction cost: {} required",
+            required
+        );
+        let remaining_collected_fees = collected_fees.amount() - fee_summary.total_royalty_cost_xrd /* royalty already distributed */;
+        let to_distribute = tips_to_distribute + fees_to_distribute;
+        assert!(
+            to_distribute == remaining_collected_fees,
+            "Remaining collected fee isn't equal to amount to distribute: {} != {}",
+            remaining_collected_fees,
+            to_distribute,
         );
 
         if !tips_to_distribute.is_zero() || !fees_to_distribute.is_zero() {