fix: ensure rumor id is serialized inside the gift wrap - #164
Conversation
Call on the rumor in before sealing: in nostr 0.45 encrypts the rumor JSON before ensuring its id (nostrdevkit/nostr#1443), so wraps went out without the rumor uid=1000(catry) gid=1000(catry) grupos=1000(catry),4(adm),27(sudo),109(kvm),124(lpadmin),134(docker),138(libvirt) field and strict NIP-59 clients rejected them. Mirror the call in the test helper and add a regression test that unwraps by hand and checks the id survives the wrap.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughRumors now receive an ID before sealing. A regression test verifies that the serialized rumor ID matches its computed ID. The raw-inner test helper follows the same behavior. ChangesRumor ID finalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change ensures rumor IDs are preserved in gift-wrapped messages and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Reviewed the current head a0350f9.
The workaround is scoped correctly: wrap_message now finalizes the rumor id before GiftWrapSealBuilder serializes/encrypts it, which matches the observed nostr 0.45.1 behavior. The regression test unwraps the encrypted payload and verifies the serialized rumor carries the computed id; I also mutation-checked that the test fails when the production ensure_id() call is removed.
Local verification passed:
cargo fmt --all -- --checkcargo test nip59::tests::rumor_id_serialized_inside_wrap -- --exactcargo testcargo clippy --all-targets --all-features -- -D warnings
CI is green on the current head and I did not find blockers.
2 participants
Since the migration to nostr 0.45.0, gift-wrapped rumors go out without the
idfield:GiftWrapSealBuilderencrypts the rumor JSON before ensuring its id, so the id computed insidebuild_sealis silently discarded. This is a bug in nostr itself, reported in nostrdevkit/nostr#1443 and fixed in nostrdevkit/nostr#1444, but until that fix ships in a release, every daemon on mostro-core ≥ 0.14.3 emits rumors that strict NIP-59 clients reject. In practice this broke the Mostro mobile app against upgraded nodes: it fails to decrypt every reply from the daemon, so orders time out client-side and trading is impossible.The workaround is a single
rumor.ensure_id()inwrap_messagebefore sealing, which restores the pre-0.45 wire format. It becomes a cheap no-op once the upstream fix lands, and it's worth keeping even then as a guard. Thewrap_with_raw_innertest helper mirrors the call so test wraps stay representative of production ones.Also adds a regression test that unwraps the gift wrap by hand, our own
unwrap_messagetolerates a missing id, so it can't catch this, and checks the rumor id survives inside the encrypted payload.Verified end to end with mostrod pointing at this branch, the mobile client can create and take orders again.
Summary by CodeRabbit
Bug Fixes
Tests