Skip to content

fix(taiko-client): run the integration tests against the PR's own protocol - #22141

Closed
dantaik wants to merge 1 commit into
claude/proposal0024-basefee-sharing-100from
claude/taiko-client-basefee-sharing-boundaries
Closed

dantaik wants to merge 1 commit into
claude/proposal0024-basefee-sharing-100from
claude/taiko-client-basefee-sharing-boundaries

Conversation

@dantaik

@dantaik dantaik commented Sep 16, 2026

Copy link
Copy Markdown
Member

Stacked on #22127 — review that one first; this PR's diff is the three files below.

The bug

taiko-client--test.yml ran its integration tests against contracts from a second checkout pinned to main, not the contracts in the pull request:

- name: Checkout protocol for testing
  uses: actions/checkout@v7
  with:
    repository: taikoxyz/taiko-mono
    path: ${{ env.PROTOCOL_FORK_DIR }}
    ref: main                                    # <-- not the PR
...
  PROTOCOL_DIR: ${{ github.workspace }}/${{ env.PROTOCOL_FORK_DIR }}/packages/protocol

Two consequences. A protocol change could never be validated against the client in the same PR — it only surfaced after merging, in some unrelated PR's CI. And because main moves, the job tested different contracts on every re-run, so a green result did not mean the same thing twice.

taiko-client-rs--test.yml already deploys the PR's own packages/protocol, with a comment giving exactly this reasoning. This brings the Go workflow in line.

What changed

  • taiko-client--test.yml — drop the main-pinned checkout and PROTOCOL_FORK_DIR; point PROTOCOL_DIR at ${{ github.workspace }}/packages/protocol, which the job already has checked out.
  • Both workflows — trigger on packages/protocol/contracts/** and packages/protocol/script/layer1/core/**. These are what the tests deploy through DeployProtocolOnL1, so a change to them can break the client without touching a single client file. Without this the fix above would still never fire on a protocol-only change.
  • bindings/encoding/input_test.go (new) — boundary coverage for basefeeSharingPctg.

basefeeSharingPctg at 0 and 100

The inbox accepts 0 <= basefeeSharingPctg <= 100 (LibInboxSetup.validateConfig), and taiko-geth splits each transaction's basefee as gasUsed * baseFee * pctg / 100 to the coinbase with the remainder to the treasury. So 0 pays the coinbase nothing and 100 pays the treasury nothing — both ends are reachable configurations, not edge cases.

The client itself is already indifferent: it only plumbs the byte into extraData and never does arithmetic with it. The exposure was entirely in tests that assumed a split. EncodeShastaExtraData had no test at all, so this adds three:

  • every value in {0, 1, 25, 75, 99, 100} round-trips through core.DecodeShastaBasefeeSharingPctg;
  • the percentage and the proposal ID never read each other's bytes, including when either is zero, up to the maximum uint48 proposal ID;
  • the three proposal IDs the encoder refuses stay refused, so an out-of-range ID can't truncate into the percentage byte.

The two treasury integration tests were made percentage-aware in #22127, which also moves DevnetInbox to 100. Combined with the workflow fix here, this PR is the first run where the integration suite actually deploys and exercises basefeeSharingPctg: 100 end to end — on the base branch alone those tests still ran against main's contracts at 75.

Testing

go test ./bindings/encoding/ and go vet ./bindings/encoding/ ./driver/chain_syncer/event/ pass locally; both workflow files parse as YAML. The integration lanes on this PR are the real check for the workflow change — they should now deploy this branch's contracts rather than main's.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UhqmeYDadT95CmcUhWzc2B


Generated by Claude Code

…tocol

The integration tests deployed `taikoxyz/taiko-mono@main`'s contracts from
a second checkout rather than the contracts in the pull request, so a
change to the protocol could never be validated against the client in the
same PR: the job tested different contracts on every re-run, and a protocol
change that breaks the client only surfaced after it had merged. The
taiko-client-rs workflow already deploys the PR's own `packages/protocol`
for exactly this reason; do the same here, and trigger both workflows on
the contracts and deploy script the tests actually run through
`DeployProtocolOnL1`.

Cover both ends of the `basefeeSharingPctg` range while we are here. The
inbox accepts 0 to 100 inclusive, and taiko-geth splits a transaction's
basefee as `gasUsed * baseFee * pctg / 100` to the coinbase with the
remainder to the treasury, so 0 pays the coinbase nothing and 100 pays the
treasury nothing. The client only ever plumbs the byte -- it does no
arithmetic with it -- so the encoder is where the range is pinned:
`EncodeShastaExtraData` had no test at all and now round-trips every
boundary value through the decoder both clients rely on, asserts the
percentage and proposal ID never read each other's bytes, and pins the
three proposal IDs the encoder refuses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UhqmeYDadT95CmcUhWzc2B
@github-actions

Copy link
Copy Markdown
Contributor

🐋 DeepSeek Code Review

🔴 Critical Issues

None.

🟡 Warnings

  • The new protocol trigger paths may still be incomplete.
    packages/protocol/script/layer1/core/** only covers that one directory. If DeployProtocolOnL1 imports helper scripts/config from outside core/** (e.g. sibling script/layer1/based/**, script/layer1/devnet/**, or shared libraries), changes there can still break the client integration suite without triggering CI. Verify that every file reachable from DeployProtocolOnL1 is covered, or broaden to packages/protocol/script/**.

  • Misleading test comment.
    basefeeSharingPctgRange is documented as “covers every value the inbox accepts,” but it only contains {0, 1, 25, 75, 99, 100}. That is sample/boundary coverage, not every accepted value. Reword the comment or loop over 0–100 if the intent is full coverage.

🔵 Suggestions

  • Use require.NoError(t, err) instead of require.Nil(t, err) for error assertions.
  • Consider table-driven t.Run cases for the basefeeSharingPctgRange loop; failures would identify the exact percentage/proposal ID without extra test output.
  • packages/protocol/contracts/** is broad and will trigger the full taiko-client integration suite for changes to contracts that are not part of the L1 deployment path. Narrower paths would reduce CI noise/cost if practical.

🟢 What Looks Good

  • Removing the main-pinned checkout and using the PR’s own checked-out protocol fixes both the nondeterminism and the “protocol changes are never validated with client changes” bug.
  • The new encoding test usefully pins the zero/100 boundary behavior, independent field boundaries, and invalid proposal ID rejections.

Automatically triggered on PR update • model: deepseek-v4-pro

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.80%. Comparing base (9e580cc) to head (f0e14f4).
⚠️ Report is 1 commits behind head on claude/proposal0024-basefee-sharing-100.

Additional details and impacted files

see 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e580cc...f0e14f4. Read the comment docs.

🚀 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.

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.

2 participants