Change payer envelope to bytes field #553
Conversation
WalkthroughThis pull request refactors various envelope handling workflows across the codebase. It introduces a new variable for unmarshalling the payer envelope in tests, enhances error handling in envelope construction within service methods, and transitions envelope creation to operate on byte representations. Modifications span API service functions, envelope utility methods, and indexing and registrant logic, ensuring that errors are caught earlier and that envelope data is consistently serialized and processed. Changes
Sequence Diagram(s)sequenceDiagram
participant APIService
participant EnvelopeBuilder
participant BlockchainPublisher
APIService->>EnvelopeBuilder: buildUnsignedOriginatorEnvelopeFromChain(targetOriginator, sequenceID, clientEnvelope)
alt Error in envelope construction
EnvelopeBuilder-->>APIService: error
APIService-->>APIService: Return error response
else Successfully constructed envelope
EnvelopeBuilder-->>APIService: envelope bytes
APIService->>BlockchainPublisher: publishToBlockchain(envelope bytes)
BlockchainPublisher-->>APIService: success response
end
sequenceDiagram
participant Storer
participant EnvelopeBuilder
participant Signer
Storer->>EnvelopeBuilder: buildOriginatorEnvelope(seqID, message)
alt Error during envelope building
EnvelopeBuilder-->>Storer: error
Storer-->>Storer: Log error and return NewLogStorageError
else Envelope built successfully
EnvelopeBuilder-->>Storer: unsigned originator envelope
Storer->>Signer: buildSignedOriginatorEnvelope(envelope)
Signer-->>Storer: signed envelope
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
🧹 Nitpick comments (3)
pkg/api/payer/service.go (3)
273-285: Include additional context in error messages
When returning an error frombuildUnsignedOriginatorEnvelopeFromChain, consider logging theSequenceIdanddesiredOriginatorIdfor richer diagnostics.
298-309: Use consistent error logging with context
As in the previous block, consider including details likeSequenceIdanddesiredOriginatorIdin the error logs for easier troubleshooting.
369-385: Use chain timestamp instead of local time
Currently,time.Now().UnixNano()is used, which might diverge from on-chain data. Consider adopting the block timestamp or another chain-derived time source for better consistency if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/proto/xmtpv4/envelopes/envelopes.pb.gois excluded by!**/*.pb.go,!pkg/proto/**
📒 Files selected for processing (10)
dev/gen-protos(1 hunks)pkg/api/message/publish_test.go(1 hunks)pkg/api/payer/service.go(3 hunks)pkg/envelopes/payer.go(1 hunks)pkg/envelopes/unsignedOriginator.go(1 hunks)pkg/indexer/storer/groupMessage.go(1 hunks)pkg/indexer/storer/identityUpdate.go(2 hunks)pkg/registrant/registrant.go(1 hunks)pkg/registrant/registrant_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (13)
dev/gen-protos (1)
4-4:Details
❓ Verification inconclusive
Branch change for protobuf generation.
The source branch for protobuf generation has been changed from the main branch to
nm/serialize-payer-envelope. This aligns with the overall PR objective of changing the payer envelope to a bytes field.You should ensure that the
nm/serialize-payer-envelopebranch on the proto repository has been merged or is stable before relying on it for builds. Otherwise, there's a risk of inconsistency if the branch is later modified or deleted.
🏁 Script executed:
#!/bin/bash # Check if the branch exists in the repo gh api repos/xmtp/proto/branches/nm/serialize-payer-envelope --silent || echo "Warning: Branch nm/serialize-payer-envelope does not exist or is not accessible"Length of output: 71
Action Required: Verify the stability of the
nm/serialize-payer-envelopebranchThe shell script didn’t output any error, which indicates that the branch likely exists, but it doesn’t confirm its stability or merge status. Please manually verify that this branch is stable (or has been merged as needed) before relying on it for protobuf generation.
- The branch was detected by the CLI check.
- Manual confirmation of the branch’s stability is still required to avoid potential inconsistencies.
pkg/envelopes/payer.go (1)
36-42: LGTM: New helper function for deserializing PayerEnvelope from bytes.This new function provides a convenient way to unmarshal bytes into a PayerEnvelope, which is essential for the transition to using byte representation instead of direct objects.
pkg/testutils/envelopes/envelopes.go (1)
121-129: Update to use serialized PayerEnvelope.The code now correctly serializes the PayerEnvelope to bytes before storing it in the UnsignedOriginatorEnvelope, consistent with the move to byte representation.
pkg/indexer/storer/groupMessage.go (1)
71-75: Good error handling improvement.Extracting the
buildOriginatorEnvelopecall and adding explicit error handling before proceeding tobuildSignedOriginatorEnvelopemakes the code more robust and easier to debug. This improvement isolates the envelope building step and ensures errors are caught and handled earlier in the process.pkg/api/message/publish_test.go (2)
49-53: Consistent with new envelope bytes approach.The added code properly unmarshals the payer envelope from bytes, aligning with the architectural change to use byte representations for envelopes throughout the system.
58-58: Updated to use the new payerEnv structure.This change correctly obtains the unsigned client envelope from the newly created payerEnv instead of the previous approach, maintaining consistency with the architectural changes.
pkg/envelopes/unsignedOriginator.go (2)
26-26: Updated to use byte representation for PayerEnvelope.This change properly implements the architectural shift to use byte representations for envelopes, using the new
NewPayerEnvelopeFromBytesfunction instead of direct unmarshalling.
34-36: Good addition of accessor method for byte data.Adding the
PayerEnvelopeBytes()method provides a clean way to access the raw byte representation, which is necessary for the new envelope handling approach throughout the codebase.pkg/registrant/registrant.go (1)
88-88: Simplified envelope handling.This change directly assigns the raw payer envelope bytes instead of unmarshalling it into an object, which is consistent with the architectural changes across the codebase to use byte representations for envelopes. This approach simplifies the code by removing unnecessary unmarshalling steps.
pkg/indexer/storer/identityUpdate.go (4)
158-162: Well-structured error handling
These lines appropriately capture the error frombuildOriginatorEnvelopeand mark it as non-retriable, which aligns with the existing error-handling pattern.
165-165: No functional change
The change on this line merely passes the computedoriginatorEnvelopeparameter within the function call, so there's nothing additional to address here.
170-170: Confirm retriability of signature building failures
The code marks this error as non-retriable. If signature failures could be transient (e.g. due to ephemeral system conditions), consider returning a retriable error instead.
253-266: Consistent approach to returning payer envelope bytes
Embedding thePayerEnvelopewithinUnsignedOriginatorEnvelopeis a clear improvement. The usage oftime.Now().UnixNano()is acceptable, but be mindful if deterministic timestamps or alignment with on-chain block times become necessary.
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
from
375c3e7 to
692804f
Compare
|
I can handle the libxmtp stuff |
|
a good practice PR to remind ourselves how to nuke the DB :) We now have some experience with retool, so |
|
If we change the |
neekolas
force-pushed
the
02-24-track_unsettled_usage
branch
from
3e0878f to
e53e8d3
Compare
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
from
692804f to
fd653e2
Compare
neekolas
force-pushed
the
02-24-track_unsettled_usage
branch
from
e53e8d3 to
58762d6
Compare
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
from
fd653e2 to
47365e0
Compare
neekolas
changed the base branch from
02-24-track_unsettled_usage
to
graphite-base/553
neekolas
force-pushed
the
graphite-base/553
branch
from
58762d6 to
c8dc2a5
Compare
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
2 times, most recently
from
3c18227 to
134c35d
Compare
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
2 times, most recently
from
654520e to
134c35d
Compare
neekolas
force-pushed
the
02-25-convert_payer_envelope_to_bytes
branch
from
134c35d to
3fa2947
Compare
TL;DR
Updated the PayerEnvelope field in UnsignedOriginatorEnvelope to use serialized bytes instead of a nested message.
Warnings
What changed?
PayerEnvelopefield toPayerEnvelopeBytesin UnsignedOriginatorEnvelope proto messageNewPayerEnvelopeFromBytesto handle deserializationHow to test?
Why make this change?
This change improves message handling by serializing the PayerEnvelope rather than nesting it directly. This provides better encapsulation and potentially more efficient processing of envelopes in the system.
Summary by CodeRabbit
New Features
Bug Fixes
Tests