Skip token sweep #426
Skip token sweep #426
Conversation
WalkthroughThe pull request adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/index.ts (1)
321-357: Critical: Missing skip logic in sweepWallet for consistency.The
sweepWalletmethod transfers ERC20 tokens without checking theskipSweepset, whileconvertHoldingsToGas(lines 432-435) does implement the skip logic. Based on the PR description stating tokens should be skipped "during sweep and conversion operations," this appears to be an inconsistency.Should
sweepWalletalso skip tokens that are inappOptions.skipSweep? If so, add similar logic here:🔎 Proposed fix to add skip logic
// sweep erc20 tokens from the wallet for (const [, tokenDetails] of this.state.watchedTokens) { report.setAttr(`details.transfers.${tokenDetails.symbol}.token`, tokenDetails.address); + + if (this.state.appOptions.skipSweep.has(tokenDetails.address.toLowerCase())) { + report.setAttr(`details.transfers.${tokenDetails.symbol}.status`, "skipped"); + continue; + } + try {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
config.env.yaml(1 hunks)config.example.yaml(1 hunks)src/cli/commands/sweep.ts(1 hunks)src/config/yaml.test.ts(4 hunks)src/config/yaml.ts(2 hunks)src/wallet/index.test.ts(2 hunks)src/wallet/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T00:57:36.977Z
Learnt from: rouzwelt
Repo: rainlanguage/rain.solver PR: 408
File: src/core/modes/index.ts:163-194
Timestamp: 2025-10-10T00:57:36.977Z
Learning: In the rain.solver repository's `src/core/modes/index.ts`, the `getEnabledTrades` function implements "opt-in per orderbook with default-all fallback" semantics: if an orderbook address is not present in any of the orderbookTradeTypes sets (router, intraOrderbook, interOrderbook), all trade types are enabled by default. If the orderbook appears in at least one set, only the trade types for which it is explicitly listed are enabled. This is intentional behavior to provide a sane default.
Applied to files:
src/cli/commands/sweep.tssrc/config/yaml.test.ts
🔇 Additional comments (8)
config.env.yaml (1)
34-34: LGTM! Configuration field added correctly.The
skipSweepfield is properly placed and follows the existing pattern of using environment variables.config.example.yaml (1)
109-112: LGTM! Example configuration is clear and well-documented.The
skipSweepconfiguration is properly documented with clear examples that follow the established pattern used throughout the configuration file.src/cli/commands/sweep.ts (1)
122-122: Verify intentional behavior: sweep command always uses empty skipSweep.The sweep command initializes
skipSweepas an empty Set, meaning no tokens will be skipped during the sweep operation. This appears intentional since this is an explicit sweep command rather than automatic bounty processing, but please confirm:
- Is it intentional that the sweep command doesn't respect a
skipSweepconfiguration?- Should users be able to specify tokens to skip even in the explicit sweep command?
src/config/yaml.ts (2)
100-101: LGTM! Type definition is clear and accurate.The
skipSweepfield is properly typed asSet<string>with clear documentation explaining its purpose for skipping bounty tokens during sweeping.
280-283: LGTM! Configuration resolution is implemented correctly.The
skipSweepfield is resolved usingValidator.resolveAddressSet, which provides:
- Address validation
- Deduplication via Set
- Clear error messaging
This is consistent with how
orderbookTradeTypesaddresses are resolved elsewhere in the codebase.src/config/yaml.test.ts (2)
42-44: LGTM! Test coverage for skipSweep configuration.The test properly verifies that
skipSweepis parsed from YAML into a Set of addresses, with appropriate test addresses following the established pattern.Also applies to: 150-150
190-190: LGTM! Test verifies default behavior.The test correctly verifies that when
skipSweepis undefined in the input, it defaults to an empty Set, ensuring the field is always initialized.Also applies to: 326-326
src/wallet/index.ts (1)
432-435: LGTM! Skip logic implemented correctly.The
convertHoldingsToGasmethod properly checks if a token address is in theskipSweepset before attempting conversion. The implementation:
- Uses
toLowerCase()for case-insensitive address comparison- Sets clear "skipped" status in the report
- Continues to the next token without attempting conversion
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.