Update `TransactionPreviewResponse` in Core API schema #1043

lrubasze · 2024-12-18T17:25:43Z

Summary

Deprecate and mark as not-required instruction_resource_changes field in TransactionPreviewResponse.

… in TransactionPreviewResponse

lrubasze · 2024-12-18T17:34:18Z

core-rust/core-api-server/core-api-schema.yaml

-            A typical client of this API is not expected to use this receipt. The primary clients 
-            this receipt is intended for is the Radix wallet or any client that needs to perform 
+            A typical client of this API is not expected to use this receipt. The primary clients
+            this receipt is intended for is the Radix wallet or any client that needs to perform
            execution summaries on their transactions.
        instruction_resource_changes:


Although this field is no longer required, it is currently always included in the TransactionPreviewResponse within the transaction preview handler.
It might be more logical to introduce an instruction_resource_changes flag in the TransactionPreviewResponseOptions to control whether instruction_resource_changes should be included in the TransactionPreviewResponse.
@dhedey WDYT?

Personally I'd rather not introduce a flag if we're deprecating it, because the flag would have to default to true anyway for backwards compatibility, so we don't get any real use out of it, it will just be a pain because we'll have to have a deprecated flag for another protocol update after the feature is removed :)

codecov-commenter · 2024-12-18T17:53:32Z

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 40.7%. Comparing base (5a4f161) to head (cc676be).

Files with missing lines	Patch %	Lines
...-api-server/src/mesh_api/conversions/operations.rs	0.0%	5 Missing ⚠️
...pi-server/src/mesh_api/handlers/account_balance.rs	0.0%	5 Missing ⚠️
...st/core-api-server/src/core_api/conversions/lts.rs	0.0%	4 Missing ⚠️
...erver/src/mesh_api/handlers/mempool_transaction.rs	0.0%	3 Missing ⚠️
...i/generated/models/transaction_preview_response.rs	0.0%	2 Missing ⚠️
...erver/src/core_api/handlers/transaction_preview.rs	0.0%	2 Missing ⚠️
...-api-server/src/mesh_api/conversions/addressing.rs	0.0%	1 Missing ⚠️
core-rust/mesh-api-server/src/mesh_api/errors.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #1043     +/-   ##
===========================================
- Coverage       40.7%   40.7%   -0.1%     
+ Complexity      4629    4628      -1     
===========================================
  Files           1892    1892             
  Lines          57683   57683             
  Branches        1527    1527             
===========================================
- Hits           23531   23529      -2     
- Misses         33672   33675      +3     
+ Partials         480     479      -1

Flag	Coverage Δ
rust	`40.7% <0.0%> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...-api-server/src/mesh_api/conversions/addressing.rs	`13.1% <0.0%> (ø)`
core-rust/mesh-api-server/src/mesh_api/errors.rs	`0.0% <0.0%> (ø)`
...i/generated/models/transaction_preview_response.rs	`0.0% <0.0%> (ø)`
...erver/src/core_api/handlers/transaction_preview.rs	`0.0% <0.0%> (ø)`
...erver/src/mesh_api/handlers/mempool_transaction.rs	`0.0% <0.0%> (ø)`
...st/core-api-server/src/core_api/conversions/lts.rs	`0.0% <0.0%> (ø)`
...-api-server/src/mesh_api/conversions/operations.rs	`0.0% <0.0%> (ø)`
...pi-server/src/mesh_api/handlers/account_balance.rs	`0.0% <0.0%> (ø)`

... and 2 files with indirect coverage changes

sonarqubecloud · 2024-12-18T17:56:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2024-12-18T17:57:27Z

Docker tags
docker.io/radixdlt/private-babylon-node:pr-1043
docker.io/radixdlt/private-babylon-node:b45c93d3c3
docker.io/radixdlt/private-babylon-node:sha-b45c93d

dhedey

Nice work, thanks

dhedey · 2024-12-20T16:48:31Z

core-rust/core-api-server/core-api-schema.yaml

-            A typical client of this API is not expected to use this receipt. The primary clients 
-            this receipt is intended for is the Radix wallet or any client that needs to perform 
+            A typical client of this API is not expected to use this receipt. The primary clients
+            this receipt is intended for is the Radix wallet or any client that needs to perform
            execution summaries on their transactions.
        instruction_resource_changes:


Personally I'd rather not introduce a flag if we're deprecating it, because the flag would have to default to true anyway for backwards compatibility, so we don't get any real use out of it, it will just be a pain because we'll have to have a deprecated flag for another protocol update after the feature is removed :)

dhedey · 2024-12-20T16:49:25Z

core-rust/core-api-server/core-api-schema.yaml

            execution summaries on their transactions.
        instruction_resource_changes:
          type: array
+          description: |
+            This object holds changes in resource balances for all vaults within affected


It's probably worth saying it does not include recalls, and should not be relied on for a comprehensive view of balance changes. Instead, the receipt balance changes should be used.

dhedey · 2024-12-20T16:51:50Z

core-rust/mesh-api-server/src/mesh_api/handlers/mempool_transaction.rs

@@ -28,10 +28,10 @@ pub(crate) async fn handle_mempool_transaction(
            )),
        );
    } else {
-        payload_hashes.get(0).unwrap()
+        payload_hashes.first().unwrap()


It looks like you must have run clippy at a different (more recent?) version compared to the code base.

This isn't a problem, but in future, can we create a separate PR for reformatting changes, if we want to do them? This avoids too much noise in the PR :)

radixbot · 2024-12-23T10:26:27Z

Benchmark summary 2024-12-23 10:26:26

Test case: xrd_transfer, TPS: 170
Test case: basic_swap, TPS: 135
Test case: radquest, TPS: 78

Benchmark dashboard
Node dashboard
Node JVM dashboard
Node Logs

lrubasze added 2 commits December 18, 2024 18:17

Update Core API schema - deprecate instruction_resource_changes field…

596fda1

… in TransactionPreviewResponse

Apply spotless

cc676be

lrubasze changed the title ~~Update transaction preview schema~~ Update TransactionPreviewResponse in Core API schema Dec 18, 2024

lrubasze commented Dec 18, 2024

View reviewed changes

dhedey approved these changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `TransactionPreviewResponse` in Core API schema #1043

Update `TransactionPreviewResponse` in Core API schema #1043

lrubasze commented Dec 18, 2024

lrubasze Dec 18, 2024

dhedey Dec 20, 2024

codecov-commenter commented Dec 18, 2024 •

edited

Loading

sonarqubecloud bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

dhedey left a comment

dhedey Dec 20, 2024

dhedey Dec 20, 2024

dhedey Dec 20, 2024

radixbot commented Dec 23, 2024

Update TransactionPreviewResponse in Core API schema #1043

Are you sure you want to change the base?

Update TransactionPreviewResponse in Core API schema #1043

Conversation

lrubasze commented Dec 18, 2024

Summary

lrubasze Dec 18, 2024

Choose a reason for hiding this comment

dhedey Dec 20, 2024

Choose a reason for hiding this comment

codecov-commenter commented Dec 18, 2024 • edited Loading

Codecov Report

sonarqubecloud bot commented Dec 18, 2024

Quality Gate passed

github-actions bot commented Dec 18, 2024

dhedey left a comment

Choose a reason for hiding this comment

dhedey Dec 20, 2024

Choose a reason for hiding this comment

dhedey Dec 20, 2024

Choose a reason for hiding this comment

dhedey Dec 20, 2024

Choose a reason for hiding this comment

radixbot commented Dec 23, 2024

Update `TransactionPreviewResponse` in Core API schema #1043

Update `TransactionPreviewResponse` in Core API schema #1043

codecov-commenter commented Dec 18, 2024 •

edited

Loading