Skip to content

feat: add MAX_GAS_ALLOWANCE_HBAR configuration #3865

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Neurone
Copy link
Contributor

@Neurone Neurone commented Jun 18, 2025

Description:

Some providers need to fully subsidise their users' transactions. This option is already available on the network and in the SDK by setting the max_gas_allowance parameter accordingly.

This PR enables this parameter to be set via the JSON-RPC Relay configuration as well.

Related issue(s):

closes #3880

Notes for reviewer:

Now both the brand new MAX_GAS_ALLOWANCE_HBAR and the existing GAS_PRICE_TINY_BAR_BUFFER env variables are loaded dynamically via ConfigService.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@Neurone Neurone requested review from a team as code owners June 18, 2025 22:30
@Neurone Neurone requested a review from konstantinabl June 18, 2025 22:30
@lfdt-bot
Copy link

lfdt-bot commented Jun 18, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@Neurone Neurone changed the title feat: add MAX_GAS_ALLOWANCE_HBAR configuration [DRAFT] feat: add MAX_GAS_ALLOWANCE_HBAR configuration Jun 18, 2025
@Neurone Neurone force-pushed the feature-add-maxGasAllowance branch from dbc4925 to 6039a8e Compare June 19, 2025 00:01
@Neurone Neurone changed the title [DRAFT] feat: add MAX_GAS_ALLOWANCE_HBAR configuration feat: add MAX_GAS_ALLOWANCE_HBAR configuration Jun 19, 2025
@quiet-node quiet-node added the enhancement New feature or request label Jun 20, 2025
@quiet-node quiet-node added this to the 0.70.0 milestone Jun 20, 2025
@Neurone Neurone force-pushed the feature-add-maxGasAllowance branch from 6039a8e to 6043b23 Compare June 25, 2025 16:54
natanasow
natanasow previously approved these changes Jun 26, 2025
Copy link
Contributor

@natanasow natanasow left a comment

Choose a reason for hiding this comment

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

LGTM, one question.

@simzzz simzzz self-requested a review June 26, 2025 14:13
simzzz
simzzz previously approved these changes Jun 26, 2025
@Neurone Neurone force-pushed the feature-add-maxGasAllowance branch from 6043b23 to f75eab5 Compare June 27, 2025 12:55
@Neurone Neurone dismissed stale reviews from simzzz and natanasow via 5e50536 June 27, 2025 12:57
Copy link

codecov bot commented Jun 27, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/relay/src/lib/precheck.ts 33.33% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

@@           Coverage Diff           @@
##             main    #3865   +/-   ##
=======================================
  Coverage        ?   46.06%           
=======================================
  Files           ?       83           
  Lines           ?     4821           
  Branches        ?      988           
=======================================
  Hits            ?     2221           
  Misses          ?     2262           
  Partials        ?      338           
Files with missing lines Coverage Δ
...ckages/config-service/src/services/globalConfig.ts 100.00% <ø> (ø)
packages/relay/src/lib/clients/sdkClient.ts 39.59% <100.00%> (ø)
packages/relay/src/lib/precheck.ts 48.27% <33.33%> (ø)
🚀 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.

Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

Thanks @Neurone for sending this. It looks good so far. Have a couple of questions (we can tackle them in following PRs if needed)

  • Should we include a tool example (in the tools/ folder) showcasing this feature?
  • What happens when the sender account does not exist? Before this feature, this wasn't possible because the sender need to have HBARs. Now that's not the case anymore. We should add a test for this scenario.

@@ -103,6 +103,7 @@ Unless you need to set a non-default value, it is recommended to only populate o
| `TX_DEFAULT_GAS` | "400000" | Default gas for transactions that do not specify gas. |
| `USE_ASYNC_TX_PROCESSING` | "true" | Set to `true` to enable `eth_sendRawTransaction` to return the transaction hash immediately after passing all prechecks, while processing the transaction asynchronously in the background. |
| `MAX_TRANSACTION_FEE_THRESHOLD` | "15000000" | Used to set the max transaction fee. This is the HAPI fee which is paid by the relay operator account. |
| `MAX_GAS_ALLOWANCE_HBAR` | "0" | The maximum amount, in hbars, that the relay is willing to pay to complete the transaction in case the senders don't provide enough funds to pay for their transactions. Please note, in case of full subsidized transactions, the sender must set the gas price to `0` and the relay must configure both the `GAS_PRICE_TINY_BAR_BUFFER`, to a number big enough to cover the discrepancy between the base gas price and `0`, and the `MAX_GAS_ALLOWANCE_HBAR` parameters, with the max amount of hbars the relay is willing to spend to execute the transaction. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about setting GAS_PRICE_TINY_BAR_BUFFER in a fully subsidized JSON-RPC Relay. It seems GAS_PRICE_TINY_BAR_BUFFER has a different purpose. We might need to change the precheck to account for the MAX_GAS_ALLOWANCE_HBAR variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MAX_GAS_ALLOWANCE_HBAR is set by the operator, so I'm not sure it needs a dedicated pre-check.

About GAS_PRICE_TINY_BAR_BUFFER, you are right, that serve a different purpose, and that's fine.

That parameter is mentioned here only as a reference, because in case the user set gasPrice=0, a low buffer value (that is probably what you want in a classic JSON-RPC relay configuration) will block those transactions during the pre-check.

So, in case an operator want to support gasPrice=0 transactions, it must update the GAS_PRICE_TINY_BAR_BUFFER parameter also accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, in case an operator want to support gasPrice=0 transactions, it must update the GAS_PRICE_TINY_BAR_BUFFER parameter also accordingly.

Exactly. I believe this is not the best UX for operators, as we are requiring them to update the unrelated GAS_PRICE_TINY_BAR_BUFFER variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can think about improving the operators UX in general, but probably just providing a preset of config values for the different use cases they want to achieve is enough.

The GAS_PRICE_TINY_BAR_BUFFER is mentioned in the comments because it's involved in the process, but it's not a constraint to set it to a specific value. Operators can still decide to keep that value small as they prefer, and pay only for txs which costs is in that specific cost range.

BTW, the scope of this PR is very limited: it's just enable operators setting the MAX_GAS_ALLOWANCE_HBAR value as they prefer, like they do for other parameters, so I would suggest to postpone further improvements for another PR.

@Neurone
Copy link
Contributor Author

Neurone commented Jul 3, 2025

  • Should we include a tool example (in the tools/ folder) showcasing this feature?

I prefer to merge this and then thinking about specific tools, but at least the new test cases already show how you can play with this parameter and obtain a fully subsidised transaction.

  • What happens when the sender account does not exist? Before this feature, this wasn't possible because the sender need to have HBARs. Now that's not the case anymore. We should add a test for this scenario.

I'm not sure I get the question. This parameter does not introduce a new feature, it just give the relay operators the ability to set that value as they prefer. In case they set a big enough value, and the sender set gasPrice to 0, the total cost of the transaction (network fees and gas consumption) will be on the operator, and nothing will be charged to the sender (here an example).

There's no other change in the transaction, the sender (the transaction's signer) is still there, untouched as usual.

@Neurone Neurone force-pushed the feature-add-maxGasAllowance branch from 05b38f2 to c67cfc0 Compare July 3, 2025 13:51
Signed-off-by: Giuseppe Bertone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAX_GAS_ALLOWANCE tracking issue
6 participants