Skip to content

Set up fees for WASM processing #5393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 24, 2025
Merged

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Apr 8, 2025

High Level Overview of Change

This PR:

  • Adds a new fee parameter, gasPrice - this is the price of 1 gas in micro-drops (i.e. 1,000,000 of this unit = 1 drop/gas)
  • Adds a new EscrowFinish parameter, ComputationAllowance - this is how much gas the transaction submitter is willing to pay
  • Adds ComputationAllowance * gasPrice / 100000 to the EscrowFinish transaction fee
  • Adds tests for everything

Context of Change

Setting up proper fees for WASM usage

Type of Change

  • New feature (non-breaking change which adds functionality)

Test Plan

Tests were added and work properly.

Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (ripple/smart-escrow@177cdaf). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/xrpld/app/tx/detail/Escrow.cpp 90.3% 3 Missing ⚠️
src/xrpld/app/ledger/Ledger.cpp 85.7% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##             ripple/smart-escrow   #5393   +/-   ##
=====================================================
  Coverage                       ?   78.1%           
=====================================================
  Files                          ?     794           
  Lines                          ?   68495           
  Branches                       ?    8303           
=====================================================
  Hits                           ?   53502           
  Misses                         ?   14993           
  Partials                       ?       0           
Files with missing lines Coverage Δ
include/xrpl/protocol/Fees.h 100.0% <ø> (ø)
include/xrpl/protocol/TER.h 100.0% <ø> (ø)
include/xrpl/protocol/detail/ledger_entries.macro 100.0% <ø> (ø)
include/xrpl/protocol/detail/transactions.macro 100.0% <ø> (ø)
src/libxrpl/protocol/STValidation.cpp 82.4% <ø> (ø)
src/libxrpl/protocol/TER.cpp 100.0% <ø> (ø)
src/xrpld/app/misc/FeeVoteImpl.cpp 95.0% <100.0%> (ø)
src/xrpld/app/misc/NetworkOPs.cpp 69.6% <100.0%> (ø)
src/xrpld/app/tx/detail/Change.cpp 69.9% <100.0%> (ø)
src/xrpld/core/Config.h 85.7% <ø> (ø)
... and 3 more

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mvadari mvadari marked this pull request as ready for review April 9, 2025 15:51
@mvadari mvadari requested review from pwang200 and oleks-rip April 9, 2025 15:59
@@ -37,6 +37,7 @@ struct Fees
std::uint32_t extensionComputeLimit{
0}; // Extension compute limit (instructions)
std::uint32_t extensionSizeLimit{0}; // Extension size limit (bytes)
std::uint32_t gasPrice{0}; // price of WASM gas (micro-drops)
Copy link
Collaborator

@oleks-rip oleks-rip Apr 9, 2025

Choose a reason for hiding this comment

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

All these names doesn't show any relations among them. Consider to give them some common root / field of usage, for example

extensionSizeLimit-> contractSizeLimit
extensionComputeLimit -> contractGasLimit
gasPrice -> contractGasPrice
ComputationAllowance -> contractGasPayment
sfFinishFunction -> sfContract
tefNO_WASM -> tefNO_CONTRACT

and so on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extensions != contracts, so that's an incorrect statement. More specifically, we may want different values for extensions vs contracts further down the line.

Copy link
Collaborator

@oleks-rip oleks-rip Apr 10, 2025

Choose a reason for hiding this comment

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

What "extension" is refer to? And as we have gasPrice, which is definitely used with contracts, not extensions, so it just bring more ambiguity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@pwang200 pwang200 left a comment

Choose a reason for hiding this comment

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

👍
nits for you to consider

  • missing 0 in "high level overview of change": " 100,000 of this unit = 1 drop/gas"
  • "gas_price = 2000000 # 2 drops per gas", I suggest to low the price to 0.1 drop per gas.

@mvadari mvadari merged commit f37d52d into XRPLF:ripple/smart-escrow Apr 24, 2025
20 of 22 checks passed
@mvadari mvadari deleted the txq branch April 29, 2025 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants