Skip to content

refactor(runtime): use lamports per signature constant - #1732

Open
leviathanbeak wants to merge 7 commits into
Syndica:mainfrom
leviathanbeak:refactor/1133-fee-handling
Open

refactor(runtime): use lamports per signature constant#1732
leviathanbeak wants to merge 7 commits into
Syndica:mainfrom
leviathanbeak:refactor/1133-fee-handling

Conversation

@leviathanbeak

@leviathanbeak leviathanbeak commented Jul 15, 2026

Copy link
Copy Markdown

reopening #1586

I missed the notification for #1586, here's the updated version (didn't have the privileges to reopen the closed one directly)

thanks, for the review

@github-project-automation github-project-automation Bot moved this to 🏗 In progress in Sig Jul 15, 2026
@leviathanbeak
leviathanbeak force-pushed the refactor/1133-fee-handling branch from c4cfbae to d205d4f Compare July 15, 2026 11:26
@dnut

dnut commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

I'm reopening the other one to keep the discussion history unified

@dnut dnut closed this Jul 15, 2026
@github-project-automation github-project-automation Bot moved this from 🏗 In progress to ✅ Done in Sig Jul 15, 2026
@dnut

dnut commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

never mind, github won't allow it because the branch was force pushed or recreated

@dnut dnut reopened this Jul 15, 2026
@github-project-automation github-project-automation Bot moved this from ✅ Done to 🟢 Ready in Sig Jul 15, 2026
@dnut
dnut requested a review from Copilot July 15, 2026 14:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors fee handling by introducing a shared LAMPORTS_PER_SIGNATURE constant in the runtime fee-check logic and replacing several hard-coded 5000 usages with that constant (or with a derived lamports-per-signature value where already available), while also simplifying SvmGateway initialization by removing a now-unneeded lamports_per_signature init parameter.

Changes:

  • Add sig.runtime.check_transactions.LAMPORTS_PER_SIGNATURE and use it in runtime execution/tests and mock transfer logic.
  • Remove lamports_per_signature from SvmGateway.Params and update all init call sites accordingly.
  • Update fee computation in RPC getFeeForMessage to use the blockhash-queue-derived lamports-per-signature instead of a hard-coded value.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/transaction_sender/MockTransferService.zig Replace hard-coded transfer fee with runtime LAMPORTS_PER_SIGNATURE.
src/rpc/hook_contexts/SendTransaction.zig Update SvmGateway.init call after removing lamports_per_signature param.
src/rpc/hook_contexts/Account.zig Use bq_lps (blockhash queue lamports-per-signature) when computing FeeDetails.
src/replay/svm_gateway.zig Remove lamports_per_signature from gateway params; set env fee rate from runtime constant.
src/replay/execution.zig Update replay/test scaffolding to match new SvmGateway.Params.
shared/runtime/transaction_execution.zig Thread lamports-per-signature into fee payer checking; update tests to use constant.
shared/runtime/check_transactions.zig Add LAMPORTS_PER_SIGNATURE constant; accept lamports_per_signature as an input to fee payer checks; update tests.
conformance/src/txn_execute.zig Use runtime constant for transaction execution environment fee configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/replay/svm_gateway.zig Outdated
.last_lamports_per_signature = last_lamports_per_signature,

.lamports_per_signature = self.params.lamports_per_signature,
.lamports_per_signature = sig.runtime.check_transactions.LAMPORTS_PER_SIGNATURE,

@leviathanbeak leviathanbeak Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment thread conformance/src/txn_execute.zig Outdated
.next_lamports_per_signature = last_lamports_per_signature,
.last_lamports_per_signature = last_lamports_per_signature,
.lamports_per_signature = 5000,
.lamports_per_signature = sig.runtime.check_transactions.LAMPORTS_PER_SIGNATURE,

@leviathanbeak leviathanbeak Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment on lines 241 to 245
&compute_budget_limits,
maybe_nonce_info,
env.rent_collector,
env.lamports_per_signature,
)) {

@leviathanbeak leviathanbeak Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@leviathanbeak
leviathanbeak force-pushed the refactor/1133-fee-handling branch from 7b2b123 to c7d526e Compare July 15, 2026 16:36
@dnut dnut moved this from 🟢 Ready to 👀 In review in Sig Jul 23, 2026
@dnut

dnut commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Hey @leviathanbeak, any thoughts on this feedback from the original PR? #1586 (review)

The removal of lamports_per_signature as an field/argument may not be something we want to do given the current solana spec.

Keeping the fields/arguments wouldn't be just because agave is currently doing it as well as using a constant variable, but more so because their FeeRateGovernor struct definition the doc comment on lamports_per_signature mentions that this amount can change over time.

A change to how this is calculated, could ultimately lead to needing to re-thread the parameter through each caller and function.

Replacing the hardcoded values with the new LAMPORTS_PER_SIGNATURE is definitely something we should keep here, since it eliminates the use of a magic number.

Also it looks like you have some ci failures.

@leviathanbeak

Copy link
Copy Markdown
Author

Hey @leviathanbeak, any thoughts on this feedback from the original PR? #1586 (review)

The removal of lamports_per_signature as an field/argument may not be something we want to do given the current solana spec.
Keeping the fields/arguments wouldn't be just because agave is currently doing it as well as using a constant variable, but more so because their FeeRateGovernor struct definition the doc comment on lamports_per_signature mentions that this amount can change over time.
A change to how this is calculated, could ultimately lead to needing to re-thread the parameter through each caller and function.
Replacing the hardcoded values with the new LAMPORTS_PER_SIGNATURE is definitely something we should keep here, since it eliminates the use of a magic number.

Also it looks like you have some ci failures.

thanks @dnut , I think everything from original comment is resolved now, will check if there are further CI issues related to this PR

@leviathanbeak
leviathanbeak force-pushed the refactor/1133-fee-handling branch from 460fa89 to b93ecff Compare July 23, 2026 15:14
@leviathanbeak

Copy link
Copy Markdown
Author

So given the feedback on the closed PR, all in all this PR

  • addresses part of Simplify fee handling #1133 by replacing hardcoded 5000 fee values with LAMPORTS_PER_SIGNATURE.
  • keeps lamports_per_signature plumbing where it already exists per review feedback
  • checkFeePayer still uses the default fee-structure value rather than blockhash/nonce fee rates.

@dnut dnut added the external label Jul 27, 2026
@dnut

dnut commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

the files were moved so you'll need to resolve the conflicts before we can merge this

@leviathanbeak
leviathanbeak force-pushed the refactor/1133-fee-handling branch from 25021a0 to cb772ef Compare July 30, 2026 13:17
@leviathanbeak

Copy link
Copy Markdown
Author

the files were moved so you'll need to resolve the conflicts before we can merge this

@dnut done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

3 participants