-
Notifications
You must be signed in to change notification settings - Fork 5
refactor: use sablier version comparison utilities #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis PR refactors version comparison logic across multiple TypeScript files to use dedicated helper utilities ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes This change spans four modified TypeScript files with a consistent refactoring pattern but applied in different contexts (fees, deprecation, proxender effects, entity streams). While the pattern is homogeneous, each file requires separate semantic verification due to varying version thresholds and comparison directions. Potential concern exists around subtle semantic shifts (e.g., Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)envio/airdrops/helpers/fees.ts(1 hunks)envio/common/deprecated.ts(2 hunks)envio/lockup/effects/proxender.ts(2 hunks)envio/lockup/store/entity-stream.ts(4 hunks)graph/CLAUDE.md(1 hunks)package.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,yaml,yml}: Usejust prettier-writeto auto-fix formatting for Markdown and YAML files when those files change
Usejust prettier-checkto verify formatting for Markdown and YAML files
Files:
graph/CLAUDE.mdCHANGELOG.md
**/*.{js,jsx,ts,tsx,json,css,graphql,gql}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx,json,css,graphql,gql}: Usejust biome-writeto auto-fix formatting for JS/TS/JSON/CSS/GraphQL files when those files change
Usejust biome-checkto verify formatting for JS/TS/JSON/CSS/GraphQL files
Files:
envio/common/deprecated.tsenvio/airdrops/helpers/fees.tsenvio/lockup/effects/proxender.tspackage.jsonenvio/lockup/store/entity-stream.ts
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Regenerate bindings after bumping
envio,@graphprotocol/graph-ts, or@graphprotocol/graph-cliversions in package.json
Files:
package.json
🧠 Learnings (10)
📚 Learning: 2025-10-20T14:28:42.861Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: graph/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:28:42.861Z
Learning: Applies to graph/**/{airdrops,flow,lockup}/schema.graphql : Do not edit generated {indexer}/schema.graphql files directly
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/graph/*/schema.graphql : NEVER edit generated files: graph/{indexer}/schema.graphql
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/envio/*/schema.graphql : NEVER edit generated files: envio/{indexer}/schema.graphql
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/envio/analytics/analytics.graphql : For the analytics indexer, edit envio/analytics/analytics.graphql directly (no files under schema/)
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:57.899Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/lockup/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:57.899Z
Learning: Applies to envio/lockup/**/lockup.graphql : Do not edit lockup.graphql directly; it is generated.
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:27.706Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/analytics/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:27.706Z
Learning: Applies to envio/analytics/**/analytics.graphql : Edit the GraphQL schema directly in analytics.graphql within this indexer
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/**/*.graphql.ts : Implement dynamic schema generators in .graphql.ts files
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:28:42.861Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: graph/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:28:42.861Z
Learning: Applies to graph/schema/** : Modify schema sources in the schema/ directory instead of editing generated schema.graphql files
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:46.074Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/flow/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:46.074Z
Learning: Applies to envio/flow/**/flow.graphql : Do not edit flow.graphql directly
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:18.351Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/airdrops/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:18.351Z
Learning: Applies to envio/airdrops/schema/**/*.graphql : Make schema changes only within the schema/ directory
Applied to files:
graph/CLAUDE.md
🧬 Code graph analysis (4)
envio/common/deprecated.ts (1)
lib/types.ts (1)
Version(57-57)
envio/airdrops/helpers/fees.ts (2)
envio/common/deployments.ts (1)
getContractVersion(34-40)lib/types.ts (1)
Version(57-57)
envio/lockup/effects/proxender.ts (1)
lib/types.ts (1)
Version(57-57)
envio/lockup/store/entity-stream.ts (1)
lib/types.ts (1)
Version(57-57)
🔇 Additional comments (7)
graph/CLAUDE.md (1)
7-8: LGTM! Helpful documentation addition.This clarifies dependency constraints for The Graph subgraphs, which will help prevent incorrect package additions.
package.json (1)
3-3: LGTM! Version bumps align with PR objectives.Package version incremented to 1.2.0 and sablier dependency updated to 1.4.0, which provides the new semantic version comparison utilities used throughout this refactoring.
Also applies to: 14-14
envio/lockup/effects/proxender.ts (1)
3-3: LGTM! Semantically equivalent refactor.The change from
version !== Version.Lockup.V1_0toisVersionAfter(version, Version.Lockup.V1_0)is correct. Both approaches returnNOT_AVAILABLEfor versions after V1_0 (V1_1, V1_2, V2_0, etc.) while allowing proxy owner lookup only for V1_0.Also applies to: 32-34
CHANGELOG.md (1)
8-8: LGTM! Changelog properly documents the release.The 1.2.0 release entry correctly documents the sablier package upgrade to v1.4.0.
Also applies to: 13-17
envio/common/deprecated.ts (1)
46-46: Refactoring verified as semantically equivalent.The version range checks are correct:
- Airdrops: No V1_0 exists;
isVersionBefore(version, V1_3)matches only V1_1 and V1_2 ✓- Lockup: No intermediate versions between V1_2 and V2_0;
isVersionBefore(version, V2_0)matches only V1_0, V1_1, and V1_2 ✓- Flow: No V0_x versions exist;
isVersionBefore(version, V1_1)matches only V1_0 ✓The refactoring improves maintainability by using semantic version comparison instead of hardcoded lists.
envio/lockup/store/entity-stream.ts (1)
142-151: Concern about line 142 is unfounded—no intermediate Lockup versions exist between V1_2 and V2_0.Sablier Lockup releases are v1.0, v1.1, v1.2, and v2.0. The code's use of
isVersionAfter(stream.version, Version.Lockup.V1_2)is functionally equivalent to checking>= V2_0, since no V1_3 or later patch versions exist between V1_2 and V2_0. The comment saying "In v2.0 and later" is slightly imprecise but not incorrect, as the boundary effectively covers only V2_0+ in practice. The code logic is sound.Likely an incorrect or invalid review comment.
envio/airdrops/helpers/fees.ts (1)
6-6: The original review comment is incorrect and can be safely ignored.From codebase analysis:
- Supported Airdrops versions are v1.1, v1.2, v1.3, v2.0 (no v1.0 exists)
isVersionAfter(v, X)returns true when v > X (strictly after)- For all existing versions, the refactored logic
isVersionAfter(version, Version.Airdrops.V1_2)is semantically equivalent to the originalversion !== V1_1 && version !== V1_2—both return false for v1.1 and v1.2, and true for v1.3 and v2.0The refactoring improves clarity of intent without changing behavior.
e51bdcf to
d9d8651
Compare
d9d8651 to
a988fed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
envio/lockup/store/entity-stream.ts (1)
208-211: Correct the outdated comment.The comment on line 208 says "<v1.2 streams didn't have the unlock shapes" but the code checks
!isVersionBefore(stream.version, Version.Lockup.V2_0), which evaluates to true for V2_0 and later versions. This indicates that shapes were introduced in V2_0, not V1_2.Apply this diff to fix the comment:
- // Note: <v1.2 streams didn't have the unlock shapes. + // Note: <v2.0 streams didn't have the unlock shapes. if (!isVersionBefore(stream.version as Version.Lockup, Version.Lockup.V2_0)) { return { shape: stream.shape }; }
🧹 Nitpick comments (1)
envio/lockup/store/entity-stream.ts (1)
140-151: Minor: Comment could be more precise.The comment on line 140 says "In v2.0 and later" but the code checks
isVersionAfter(stream.version, Version.Lockup.V1_2). While functionally correct (since V2_0 comes after V1_2), consider updating the comment to say "In versions after v1.2" for better alignment with the code.The refactor correctly preserves the version-gating logic and is more maintainable.
Apply this diff if you want to align the comment with the code:
- // In v2.0 and later, the cliff time is set to zero if there is no cliff. + // In versions after v1.2, the cliff time is set to zero if there is no cliff. // See https://github.com/sablier-labs/lockup/blob/v2.0/src/libraries/Helpers.sol#L204-L219
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
envio/airdrops/helpers/fees.ts(1 hunks)envio/common/deprecated.ts(2 hunks)envio/lockup/effects/proxender.ts(2 hunks)envio/lockup/store/entity-stream.ts(4 hunks)graph/CLAUDE.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- envio/common/deprecated.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,css,graphql,gql}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx,json,css,graphql,gql}: Usejust biome-writeto auto-fix formatting for JS/TS/JSON/CSS/GraphQL files when those files change
Usejust biome-checkto verify formatting for JS/TS/JSON/CSS/GraphQL files
Files:
envio/airdrops/helpers/fees.tsenvio/lockup/store/entity-stream.tsenvio/lockup/effects/proxender.ts
**/*.{md,yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,yaml,yml}: Usejust prettier-writeto auto-fix formatting for Markdown and YAML files when those files change
Usejust prettier-checkto verify formatting for Markdown and YAML files
Files:
graph/CLAUDE.md
🧠 Learnings (10)
📚 Learning: 2025-10-20T14:28:42.861Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: graph/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:28:42.861Z
Learning: Applies to graph/**/{airdrops,flow,lockup}/schema.graphql : Do not edit generated {indexer}/schema.graphql files directly
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/graph/*/schema.graphql : NEVER edit generated files: graph/{indexer}/schema.graphql
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/envio/*/schema.graphql : NEVER edit generated files: envio/{indexer}/schema.graphql
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/envio/analytics/analytics.graphql : For the analytics indexer, edit envio/analytics/analytics.graphql directly (no files under schema/)
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:57.899Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/lockup/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:57.899Z
Learning: Applies to envio/lockup/**/lockup.graphql : Do not edit lockup.graphql directly; it is generated.
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:27.706Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/analytics/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:27.706Z
Learning: Applies to envio/analytics/**/analytics.graphql : Edit the GraphQL schema directly in analytics.graphql within this indexer
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:29:12.051Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: schema/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:29:12.051Z
Learning: Applies to schema/**/*.graphql.ts : Implement dynamic schema generators in .graphql.ts files
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:28:42.861Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: graph/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:28:42.861Z
Learning: Applies to graph/schema/** : Modify schema sources in the schema/ directory instead of editing generated schema.graphql files
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:46.074Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/flow/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:46.074Z
Learning: Applies to envio/flow/**/flow.graphql : Do not edit flow.graphql directly
Applied to files:
graph/CLAUDE.md
📚 Learning: 2025-10-20T14:27:18.351Z
Learnt from: CR
PR: sablier-labs/indexers#0
File: envio/airdrops/CLAUDE.md:0-0
Timestamp: 2025-10-20T14:27:18.351Z
Learning: Applies to envio/airdrops/schema/**/*.graphql : Make schema changes only within the schema/ directory
Applied to files:
graph/CLAUDE.md
🧬 Code graph analysis (3)
envio/airdrops/helpers/fees.ts (1)
envio/common/deployments.ts (1)
getContractVersion(34-40)
envio/lockup/store/entity-stream.ts (1)
lib/types.ts (1)
Version(57-57)
envio/lockup/effects/proxender.ts (1)
lib/types.ts (1)
Version(57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (7)
graph/CLAUDE.md (1)
7-8: LGTM!Clear documentation of the dependency constraint for Graph subgraphs. This aligns with the AssemblyScript environment and helps prevent introducing incompatible npm dependencies.
envio/lockup/effects/proxender.ts (2)
3-3: LGTM!Good addition of the version comparison utility import.
32-32: LGTM!The refactor from
version !== Version.Lockup.V1_0toisVersionAfter(version, Version.Lockup.V1_0)is semantically equivalent and more maintainable. Since PRBProxy was only used in V1_0, returningNOT_AVAILABLEfor all subsequent versions is correct.envio/airdrops/helpers/fees.ts (2)
1-1: LGTM!Appropriate import of the version comparison utility.
4-7: LGTM!The refactored implementation is cleaner and more maintainable. Using
isVersionAftermakes the intent explicit and automatically handles future versions after V1_2 without requiring code changes.envio/lockup/store/entity-stream.ts (2)
1-1: LGTM!Appropriate imports for version comparison utilities.
169-181: LGTM!The refactor correctly handles versions before V1_2 (V1_0 and V1_1) with proper documentation and source references. The use of
isVersionBeforeis semantically clear and maintainable.
Replaces manual version checks with semantic comparison utilities (
isVersionAfter,isVersionBefore) from[email protected]. This makes version filtering more maintainable and future-proof.Changes in 4 files:
envio/airdrops/helpers/fees.ts: UseisVersionAfterfor fee detectionenvio/common/deprecated.ts: UseisVersionBeforefor deprecation checksenvio/lockup/effects/proxender.ts: UseisVersionAfterfor proxy detectionenvio/lockup/store/entity-stream.ts: Use version comparisons for cliff/shape logicWhen new versions are added, these utilities automatically handle range checks correctly.