Skip to content

Select funded tester live stimulus - #46

Merged
phroi merged 6 commits into
masterfrom
review/tester-live-stimulus
May 27, 2026
Merged

phroi merged 6 commits into
masterfrom
review/tester-live-stimulus

Conversation

@phroi

@phroi phroi commented May 27, 2026

Copy link
Copy Markdown
Member

Why

The tester should create useful live stimulus only after current balances are known. The previous auto path could select unfunded or full-balance stress paths, fresh bot-updated descendants could extend cooldowns, and skipped transactions could be logged like committed orders.

Changes

  • Delay auto selection until balances are loaded and limit it to conservative funded scenarios.
  • Add bounded iCKB-to-CKB stimulus and choose a buildable SDK conversion direction.
  • Treat freshness cooldown as minted-order only and shorten the cooldown window.
  • Check completed transactions against the tester plain-CKB reserve for all scenarios.
  • Log attempted evidence for estimate and reserve skips instead of committed newOrder evidence.
  • Keep private-key material out of production logging helper inputs.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an 'auto' scenario selection mode, a new 'bounded-ickb-to-ckb-limit-order' scenario, and refactors the plain CKB reserve enforcement logic. Feedback on the changes highlights a critical issue where applying the CKB reserve check unconditionally to all transactions (including CKB-replenishing 'ickb-to-ckb' orders) could lead to a deadlock when the balance is below the reserve.

Comment thread apps/tester/src/index.ts
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several enhancements to the iCKB Tester, including a new 'auto' scenario selection mode that dynamically chooses funded scenarios based on current balances, a new 'bounded-ickb-to-ckb-limit-order' scenario, and stricter post-transaction plain CKB reserve enforcement. Additionally, it prevents private key leakage in crash logs and reduces the fresh matchable order block threshold. The code reviewer identified a potential deadlock issue in the reserve gate check where neutral transactions (which do not change the CKB balance) are blocked when the balance is already below the reserve threshold; changing the condition to >= was suggested to resolve this.

Comment thread apps/tester/src/index.ts Outdated
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an "auto" scenario selection mode for the iCKB tester, which dynamically selects conservative funded scenarios based on current balances. It also adds a new bounded-ickb-to-ckb-limit-order scenario, refactors plain-CKB reserve enforcement, and improves security by preventing private key leaks in crash logs. One issue was identified in the review where cell.outPoint is accessed without a defensive check, which could lead to a runtime TypeError since it can be optional.

Comment thread apps/tester/src/index.ts
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an "auto" scenario selection mode for the iCKB tester, which dynamically chooses from conservative funded scenarios based on current balances. It also adds a new bounded-ickb-to-ckb-limit-order scenario, refactors plain CKB reserve enforcement, prevents private key leakage in error logs, and reduces the block elapsed threshold for fresh matchable orders. Feedback is provided to simplify a redundant check in the fundedTesterScenarios function, as the sdk-conversion scenario is already handled by an early return.

Comment thread apps/tester/src/index.ts Outdated
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the iCKB Tester to support an auto scenario selection that dynamically chooses funded scenarios based on current balances. It introduces a new bounded-ickb-to-ckb-limit-order scenario capped at one deposit-cap unit, refactors how the post-transaction plain CKB reserve is enforced, and prevents private key leakage in crash logs. Additionally, the block threshold for fresh matchable orders is reduced from 5400 to 180 blocks. The reviewer feedback highlights a potential deadlock issue in enforceTesterPlainCkbReserve where explicit CKB reserve scenarios might still throw terminal errors even if they preserve or increase the plain CKB balance, and suggests a corresponding test update.

Comment thread apps/tester/src/index.ts Outdated
Comment thread apps/tester/src/index.test.ts

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several improvements and new features to the iCKB Tester. Key changes include the addition of a new bounded-ickb-to-ckb-limit-order scenario, the implementation of a dynamic auto scenario selection that chooses from funded scenarios based on current balances, and a reduction of MAX_ELAPSED_BLOCKS in freshMatchableOrderSkip.ts. Additionally, the PR refactors plain-CKB reserve enforcement, enhances logging for skips and actions, prevents potential private key leaks in error logs, and adds comprehensive unit tests to cover these new behaviors. I have no feedback to provide as there are no review comments.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several enhancements and bug fixes to the iCKB Tester. Key changes include reducing the MAX_ELAPSED_BLOCKS threshold, adding a new bounded-ickb-to-ckb-limit-order scenario, and implementing an auto scenario selection that dynamically chooses from funded scenarios based on current balances. Additionally, plain CKB reserve enforcement has been refactored and strengthened, and a security fix was applied to prevent the private key from leaking in crash logs. Comprehensive unit tests have been added to cover these new behaviors. I have no review comments or feedback to provide as no issues were identified.

@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several enhancements and refactorings to the iCKB tester. Key changes include reducing the maximum elapsed blocks for fresh matchable orders, adding a new bounded-ickb-to-ckb-limit-order scenario, and implementing dynamic scenario resolution for the auto mode based on current balances. Additionally, it refactors the plain-CKB reserve enforcement, improves transaction logging/evidence for skipped or attempted transactions, and prevents potential private key leaks in loop error handling. I have no feedback to provide as there are no review comments to assess.

@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

LGTM

Phroi %120

@phroi
phroi merged commit 1de1901 into master May 27, 2026
1 check passed
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.

1 participant