fix issues in `simulate` RPC #5265

mvadari · 2025-01-30T16:54:25Z

High Level Overview of Change

This PR prevents the use of seed, secret, seed_hex, and passphrase fields in the simulate RPC, and adds autofilling for NetworkIDs in simulated transactions.

Context of Change

The former is to avoid confusion with RPCs like sign and simulate. Transactions provided in the simulate RPC are supposed to be unsigned, so users should also not submit any secrets alongside those transactions.

The latter is to make it easier to work with networks with a larger network ID.

Type of Change

Bug fix (non-breaking change which fixes an issue)

API Impact

Public API: Fix

Test Plan

Tests added.

Bronek

LGTM

codecov · 2025-01-30T20:06:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.1%. Comparing base (02387fd) to head (1eb8578).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5265   +/-   ##
=======================================
  Coverage     78.1%   78.1%           
=======================================
  Files          790     790           
  Lines        67607   67619   +12     
  Branches      8164    8163    -1     
=======================================
+ Hits         52828   52842   +14     
+ Misses       14779   14777    -2

Files with missing lines	Coverage Δ
src/xrpld/rpc/detail/TransactionSign.cpp	`90.0% <100.0%> (+0.1%)`	⬆️
src/xrpld/rpc/handlers/Simulate.cpp	`100.0% <100.0%> (ø)`

... and 2 files with indirect coverage changes

oleks-rip

Looks good

mvadari · 2025-02-05T15:27:19Z

Adding another fix to this PR

Added a new nontrivial commit

mvadari · 2025-02-05T15:34:54Z

Tentative commit message:

make `simulate` RPC easier to use
- prevent the use of `seed`, `secret`, `seed_hex`, and `passphrase` fields (to avoid confusing with the signing methods)\
- add autofilling of the `NetworkID` field

src/test/rpc/JSONRPC_test.cpp

src/test/rpc/Simulate_test.cpp

Bronek

Can you pls add an item into API-CHANGELOG.md with the new behaviour of sign-and-submit ?

src/test/rpc/Simulate_test.cpp

oleks-rip · 2025-02-06T18:14:11Z

src/xrpld/rpc/detail/TransactionSign.cpp

@@ -467,6 +467,13 @@ transactionPreProcessImpl(

        if (!tx_json.isMember(jss::Flags))
            tx_json[jss::Flags] = tfFullyCanonicalSig;
+
+        if (!tx_json.isMember(jss::NetworkID))


Please move if (signingArgs.editFields()) block into separate "Autofill" function. transactionPreProcessImpl is already too big and better to split it

I have even better idea - the same "Autofill" function must be the used across "submit" and "simulate" (to produce even more consistent results).

I think that's out of scope of this particular PR - but yes, would like to do that as a future refactor.

Expanding already big function is a bad change, so I better insist on moving out autofill block. Reusing "autofill" with the "simulate" is up to you, I can't assess the complexity of this change now.

Refactoring will expand this PR from just a couple of lines changing here to being much larger, which will also require more testing etc.

src/xrpld/rpc/detail/TransactionSign.cpp

oleks-rip

ok

kuznetsss · 2025-02-07T16:36:56Z

Ok from Clio team

Make `simulate` RPC easier to use: * Prevent the use of `seed`, `secret`, `seed_hex`, and `passphrase` fields (to avoid confusing with the signing methods). * Add autofilling of the `NetworkID` field.

prevent seed fields in simulate RPC

e82c328

mvadari requested review from Bronek and oleks-rip January 30, 2025 16:54

Bronek previously approved these changes Jan 30, 2025

View reviewed changes

oleks-rip previously approved these changes Jan 30, 2025

View reviewed changes

mvadari added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Jan 30, 2025

mvadari added 2 commits February 5, 2025 07:30

additional fix for NetworkID autofill

a481bad

Merge branch 'develop' into simulate-seed

d3ca561

mvadari force-pushed the simulate-seed branch from 88a7b0e to d3ca561 Compare February 5, 2025 15:31

mvadari requested review from oleks-rip and Bronek February 5, 2025 15:31

mvadari changed the title ~~fix: prevent seed fields in simulate RPC~~ fix issues in simulate RPC Feb 5, 2025

mvadari added 2 commits February 5, 2025 10:44

add NetworkID autofill to sign/submit functions

c50f934

Merge branch 'develop' into simulate-seed

b60a3a6

mvadari mentioned this pull request Feb 5, 2025

feat: add support for the simulate RPC (XLS-69d) XRPLF/xrpl-py#793

Merged

2 tasks

Bronek reviewed Feb 5, 2025

View reviewed changes

src/test/rpc/JSONRPC_test.cpp Show resolved Hide resolved

Bronek reviewed Feb 5, 2025

View reviewed changes

src/test/rpc/Simulate_test.cpp Outdated Show resolved Hide resolved

mvadari added 2 commits February 5, 2025 11:57

add zero NetworkID

595d125

clean up tests

ec9939d

Bronek approved these changes Feb 5, 2025

View reviewed changes

Bronek self-requested a review February 5, 2025 20:29

Bronek approved these changes Feb 5, 2025

View reviewed changes

update changelog

3c2be55

mvadari added this to the 2.4.0 (Feb 2025) milestone Feb 5, 2025

oleks-rip reviewed Feb 6, 2025

View reviewed changes

src/test/rpc/Simulate_test.cpp Show resolved Hide resolved

oleks-rip reviewed Feb 6, 2025

View reviewed changes

src/xrpld/rpc/detail/TransactionSign.cpp Show resolved Hide resolved

Merge branch 'develop' into simulate-seed

1eb8578

oleks-rip approved these changes Feb 6, 2025

View reviewed changes

mvadari added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 6, 2025

bthomee merged commit d9e4009 into XRPLF:develop Feb 7, 2025
22 checks passed

mvadari deleted the simulate-seed branch February 7, 2025 20:27

This was referenced Feb 18, 2025

Proposed 2.4.0-rc1 #5295

Closed

Proposed 2.4.0-rc2 #5307

Closed

Proposed 2.4.0-rc2 #5308

Closed

This was referenced Feb 26, 2025

Proposed 2.4.0-rc3 #5322

Closed

Proposed 2.4.0-rc4 #5329

Merged

Release 2.4.0 #5332

Closed

This was referenced Mar 5, 2025

Release 2.4.0 #5334

Closed

Release 2.4.0 #5337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issues in `simulate` RPC #5265

fix issues in `simulate` RPC #5265

mvadari commented Jan 30, 2025 •

edited

Loading

Bronek left a comment

codecov bot commented Jan 30, 2025 •

edited

Loading

oleks-rip left a comment

mvadari commented Feb 5, 2025

mvadari commented Feb 5, 2025

Bronek left a comment

oleks-rip Feb 6, 2025

oleks-rip Feb 6, 2025 •

edited

Loading

mvadari Feb 6, 2025 •

edited

Loading

oleks-rip Feb 6, 2025 •

edited

Loading

mvadari Feb 6, 2025

oleks-rip left a comment

kuznetsss commented Feb 7, 2025

fix issues in simulate RPC #5265

fix issues in simulate RPC #5265

Conversation

mvadari commented Jan 30, 2025 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Bronek left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

oleks-rip left a comment

Choose a reason for hiding this comment

mvadari commented Feb 5, 2025

mvadari commented Feb 5, 2025

Bronek left a comment

Choose a reason for hiding this comment

oleks-rip Feb 6, 2025

Choose a reason for hiding this comment

oleks-rip Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

mvadari Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

oleks-rip Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

mvadari Feb 6, 2025

Choose a reason for hiding this comment

oleks-rip left a comment

Choose a reason for hiding this comment

kuznetsss commented Feb 7, 2025

fix issues in `simulate` RPC #5265

fix issues in `simulate` RPC #5265

mvadari commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading

oleks-rip Feb 6, 2025 •

edited

Loading

mvadari Feb 6, 2025 •

edited

Loading

oleks-rip Feb 6, 2025 •

edited

Loading