Skip to content

bind: fix dynamic-fee tx selection and DeployContract construction - #1689

Merged
libotony merged 5 commits into
masterfrom
pedro/fix-send-builder-tx-type
Oct 26, 2026
Merged

libotony merged 5 commits into
masterfrom
pedro/fix-send-builder-tx-type

Conversation

@otherview

@otherview otherview commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Description

Fixes three defects in the thorclient/bind SDK, all in transaction construction:

  • SendBuilder.Submit selected dynamic-fee mode when the best block has a base fee, but built the transaction with new(tx.Builder). The tx type stayed at the legacy zero value, so the fee caps were silently dropped and post-Galactica submissions paid the legacy gas price instead of the selected cap. The builder is now created with tx.NewBuilder(txType).
  • DeployContract never set BlockRef (0) with Expiration(10000), so its transaction was expired on arrival on any chain past block 10,000 — it never worked on mainnet or testnet. It also hardcoded the legacy tx type (no dynamic-fee option), left the nonce at 0 (duplicate tx IDs), and hardcoded a flat 10M gas. It now simulates the creation clause up front for strict gas estimation (IntrinsicGas + simulated GasUsed) and a pre-flight revert check, selects the tx type from the best block's base fee, anchors BlockRef to the best block, and uses a random nonce. The public signature is unchanged.
  • The ChainTag() doc example passed the chain tag to tx.NewBuilder(), which sets the tx type — it compiles (both are byte) but is wrong.

A shared clause-simulation helper (simulate.go) backs the estimation in both Submit and DeployContract; per-site policy is unchanged (Submit keeps the 15k OP_CALL buffer and no revert check; DeployContract is strict).

Client-side SDK only — no node code paths and no public API changes.

No linked issue.

Type of change

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

How Has This Been Tested?

  • go build ./... and make lint clean
  • Full ./thorclient/... suite passes; the bind tests deploy a contract through the new DeployContract path on a Galactica-enabled chain and exercise Submit end-to-end
  • During triage, a regression test asserting tx type and fee caps was verified to fail on the unfixed code (test not included in this PR)

Test Configuration:

  • Go Version: go1.26.5 darwin/arm64
  • Hardware: Apple Silicon (macOS)
  • Docker Version: N/A (in-process test node via testnode)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • New and existing E2E tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have not added any vulnerable dependencies to my code

@otherview
otherview requested a review from a team as a code owner October 13, 2026 13:17
@codecov-commenter

codecov-commenter commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.64179% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
thorclient/bind/send.go 74.19% 4 Missing and 4 partials ⚠️
thorclient/bind/simulate.go 72.41% 4 Missing and 4 partials ⚠️
thorclient/bind/contract.go 57.14% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@libotony libotony left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Direction looks good. A few non-blocking notes:

  • DeployContract now re-implements most of Submit's tx assembly: type selection, fee default, blockRef, nonce, gas estimation. The two copies already differ. A shared helper would mean the fee-cap and gas-margin fixes below are made once.
  • Pre-existing, not from this PR:
    • Submit writes the defaults back into the caller's TxOptions (opts := b.opts), so reusing a builder reuses the nonce, gas and blockRef.
    • Setting only MaxPriorityFeePerGas gets a zero effective tip, because MaxFeePerGas defaults to the base fee. The fee-cap fix covers this too.

Comment thread thorclient/bind/send.go Resolved
Comment thread thorclient/bind/contract.go Resolved
Comment thread thorclient/bind/contract.go Resolved
Comment thread thorclient/bind/send.go Resolved
…lts into TxOptions

- buildTx is the one place that fills the tx defaults for Submit and
  DeployContract: tx type, blockRef, nonce, expiration and fee caps.
- The default MaxFeePerGas is 2*baseFee + tip. The txpool and the packer
  price a tx against the next block's base fee, so a cap at the current
  base fee gets the tx rejected or washed out when the base fee rises.
  The tip is added so that a priority fee alone is still paid.
- estimateGas adds max(20% of execution gas, 15k) for clauses that run
  code. With an exact limit, the 1/64 that each nested CALL/CREATE keeps
  back (EIP-150) can run the tx out of gas.
- Submit copies TxOptions, so a reused builder no longer resends the
  same nonce, blockRef and gas.
@otherview

Copy link
Copy Markdown
Member Author

Thanks. All points are in 2d99678: the shared buildTx/estimateGas, Submit copies TxOptions, and the default cap is 2 × baseFee + tip. I added TestContract_SubmitTxConstruction. It fails on the previous code.

@libotony
libotony merged commit 47a50a0 into master Oct 26, 2026
37 checks passed
@libotony
libotony deleted the pedro/fix-send-builder-tx-type branch October 26, 2026 04:02
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.

3 participants