feat: finalize transport v2 migration (Phase C)#626
Open
AndreaDiazCorreia wants to merge 2 commits into
Open
Conversation
… and remove global version constant
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
WalkthroughRemoves the global ChangesTransport v2 migration finalization
Sequence Diagram(s)No sequence diagrams generated. The changes do not introduce new multi-component interactions or control flow modifications that would benefit from sequencing visualization; they are primarily constant removal, function extraction, UI additions, and documentation updates. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
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 |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
|
A couple of non-blocking nits I would leave as follow-up comments:
Nothing here changes my verdict; the PR still looks good to merge. |
grunch
previously approved these changes
Jun 22, 2026
Member
There was a problem hiding this comment.
Review — Phase C (finalize transport v2)
Verified by checking out the branch: regenerated code and ran flutter analyze + flutter test on the affected and related files.
Checks ✅
Config.mostroVersionremoved, no dangling references inlib/.Configimport removed frommostro_message.dart(no longer used there); still imported/used inmostro_service_test.dart(Config.keyDerivationPath) → no unused import.- v1 path (
wrap→serialize→toJson()default) yieldsversion: 1, identical to the previousConfig.mostroVersion = 1→ byte-for-byte equivalent. - v2 path (
wrapNip44→toJson(version: 2)) is correct. buildOrdersFilterreproduces the originalswitchexactly (kinds /p/authors), is well documented and annotated@visibleForTesting.- All 5 test files cited in the docs exist and pass (orders_filter, mostro_service, nip44, transport, mostro_instance, restore_decode).
Removing Config.mostroVersion is safe: it was a static int but only ever read (never mutated). The other mostroVersion in the repo (MostroInstance.mostroVersion, a String = daemon version) is semantically different and unaffected.
Minor (non-blocking)
- Docs say "Migration complete… merged" inside a still-open PR: the §Status claims Phase C is already merged, when it is precisely this PR. Cosmetic — becomes true on merge.
- Scattered magic version numbers: the "giftWrap=1 / nip44=2" mapping lives in the
?? 1default oftoJson, the literalversion: 2inwrapNip44, andresolveTransport. It works and is commented, but a single source of truth (e.g.Transport.wireVersion) would prevent future drift.
Environment note (unrelated to this PR)
I could not fully reproduce "flutter analyze: no issues": there are 2 pre-existing errors (ScrollCacheExtent in lib/features/chat/widgets/chat_messages_list.dart:119, introduced in #613, SDK-version dependent). Not attributable to this PR — worth a separate issue.
Verdict
Approved. Clean, low-risk refactor that preserves the v1 path exactly, enables the already-existing v2 path, and improves testability. Docs are accurate.
(Edited: original review text was posted in Spanish by mistake; reposting in English.)
Add protocol_version field to about screen showing the node's transport protocol (1 = NIP-59 gift wrap, 2 = NIP-44 direct messages) with explanatory dialog. Add localized strings in en/es/it/de/fr.
AndreaDiazCorreia
dismissed stale reviews from grunch and ermeme[bot]
via
0af1788
Catrya
requested changes
Jun 23, 2026
Member
There was a problem hiding this comment.
- When try to create a new order (Mostrod: anti-abuse bond = false).
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│ Bad state: Tried to rebuild StateProvider<Session?>#e0a7c() multiple times in the same frame
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ #0 ProviderScheduler.debugNotifyDidBuild (package:riverpod/src/core/scheduler.dart:181:9)
│ #1 ProviderElement.buildState (package:riverpod/src/core/element.dart:731:27)
│ #2 ProviderElement._performRebuild (package:riverpod/src/core/element.dart:638:7)
│ #3 ProviderElement.flush (package:riverpod/src/core/element.dart:705:7)
│ #4 $ProviderBaseImpl._addListener (package:riverpod/src/core/provider/provider.dart:119:24)
│ #5 ProviderContainer.listen (package:riverpod/src/core/provider_container.dart:1102:26)
│ #6 ProviderContainer.read (package:riverpod/src/core/provider_container.dart:1028:17)
│ #7 Ref.read (package:riverpod/src/core/ref.dart:609:30)
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ 00:19:28.984 (+0:17:13.715157)
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ ⛔ Error handling automatic cancellation for order
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│ Bad state: Tried to rebuild StateProvider<Session?>#81776(304867118) multiple times in the same frame
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ #0 ProviderScheduler.debugNotifyDidBuild (package:riverpod/src/core/scheduler.dart:181:9)
│ #1 ProviderElement.buildState (package:riverpod/src/core/element.dart:731:27)
│ #2 ProviderElement._performRebuild (package:riverpod/src/core/element.dart:638:7)
│ #3 ProviderElement.flush (package:riverpod/src/core/element.dart:705:7)
│ #4 $ProviderBaseImpl._addListener (package:riverpod/src/core/provider/provider.dart:119:24)
│ #5 ProviderContainer.listen (package:riverpod/src/core/provider_container.dart:1102:26)
│ #6 ProviderContainer.read (package:riverpod/src/core/provider_container.dart:1028:17)
│ #7 Ref.read (package:riverpod/src/core/ref.dart:609:30)
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ 00:19:28.986 (+0:17:13.717585)
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ ⛔ Error handling automatic cancellation for order 304867118
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
- When try to create a new order (Mostro: anti-abuse bond=true)
```
──
│ #0 getNotificationLaunchOrderId (package:mostro_mobile/features/notifications/services/background_notification_service.dart:61:12)
│ #1
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ 00:15:26.130 (+0:13:10.861509)
├┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
│ ⛔ Error checking notification launch details: UnimplementedError: getNotificationAppLaunchDetails() has not been implemented
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
```
Member
|
When try to take an order for just a few seconds a red screen apears but i hadn't time to do an screenshot. Also when try to take a new order and then go back to order book |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finalizes the transport v1 → v2 migration (
docs/architecture/TRANSPORT_V2_MIGRATION.md). The send/receive wiring already landed in Phase A (#621) and Phase B (#624); this closes the remaining loose ends.Changes
Config.mostroVersion: the messageversionis now derived from the transport —MostroMessage.toJsondefaults to1(gift wrap), andwrapNip44passes2.buildOrdersFilter(transport, tradeKeys, mostroPubkey)from the subscription manager, with unit tests (v1 → kind 1059; v2 → kind 14 pinned toauthors=[mostroPubkey]).Verification
flutter analyze: no issues.flutter test: all pass (v1 path unchanged).Summary by CodeRabbit