Fix BIP-340 signature bug by overriding bip340 to 0.2.0#496
Fix BIP-340 signature bug by overriding bip340 to 0.2.0#496
Conversation
WalkthroughAdded a dependency override for the bip340 package (version ^0.2.0) in pubspec.yaml. This explicit override modifies dependency resolution for the project, addressing signature validation issues in the Mostrod integration. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pubspec.yaml (1)
108-109: Consider pinning the exact version and documenting the override reason.The
pubspec.lockcorrectly resolvesbip340to 0.2.0 as expected. However, the caret constraint^0.2.0allows for potentially wider version ranges in pre-1.0 packages. For a targeted override addressing a specific issue, pinning the exact version and adding a brief inline comment will improve clarity and prevent accidental drift.♻️ Suggested improvement
dependency_overrides: - bip340: ^0.2.0 + # Override due to Schnorr signature padding bug in bip340 0.1.x + bip340: 0.2.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pubspec.yaml` around lines 108 - 109, Change the caret constraint in dependency_overrides to pin bip340 to the exact version used (replace "^0.2.0" with "0.2.0") and add a short inline comment explaining why the override is needed; update the dependency_overrides entry for bip340 accordingly so the lockfile cannot drift and future maintainers see the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pubspec.yaml`:
- Around line 108-109: Change the caret constraint in dependency_overrides to
pin bip340 to the exact version used (replace "^0.2.0" with "0.2.0") and add a
short inline comment explaining why the override is needed; update the
dependency_overrides entry for bip340 accordingly so the lockfile cannot drift
and future maintainers see the rationale.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pubspec.yaml
There was a problem hiding this comment.
Clean fix for a nasty bug (1-2% invalid Schnorr signatures due to missing padding in bigToBytes()).
Reviewed:
- ✅
dependency_overridesis the correct Dart mechanism for this — forcesbip340 0.2.0without waiting fordart_nostrto update - ✅ 0.2.0 has the same API as 0.1.0 (just the padding fix), so no breaking changes
- ✅ Can't use 0.3.0+ because
verify()changed fromString?toString— would breakdart_nostr's nullable parameter - ✅ CI passes (218 tests), no conflicts, mergeable
- ✅
pubspec.lockcorrectly reflects the override - ✅ PR body clearly documents the reasoning and when the override can be removed
Only note: add a # TODO: comment in pubspec.yaml above the override explaining when it can be removed, so it doesn't become permanent tech debt that everyone is afraid to touch:
# TODO: Remove override when dart_nostr updates bip340 dependency to >=0.2.0
# See: https://github.com/MostroP2P/mobile/issues/491
dependency_overrides:
bip340: ^0.2.0Not blocking — approved.
fix #491 #276
dart_nostr 9.1.1 depends on bip340 0.1.0, which has a bug in bigToBytes() that doesn't pad values to 32 bytes. This causes about 1-2% of Schnorr signatures to be invalid, and 100% failure for keys whose public key starts with 0x00.
The bug was fixed upstream in bip340 0.2.0. We can't go higher than 0.2.0 because version 0.3.0+ changed verify() from String? to String, which breaks dart_nostr's nullable parameter and won't compile.
bip340 0.2.0 has the same API as 0.1.0, just with the padding fix applied. All 218 tests pass.
The override can be removed when dart_nostr updates its bip340 dependency or when we migrate to a different nostr library.
Summary by CodeRabbit