Skip to content

Conversation

@sundayonah
Copy link
Collaborator

@sundayonah sundayonah commented Dec 21, 2025

Description

Describe the purpose of this PR along with any background information and the impacts of the proposed change. For the benefit of the community, please do not assume prior context.

Provide details that support your chosen implementation, including: breaking changes, alternatives considered, changes to the API, contracts etc.

References

Include any links supporting this change such as a:

If there are no references, simply delete this section.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this project has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation and tests for new/changed functionality in this PR
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not main

By submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.

Summary by CodeRabbit

  • Updates
    • Account generation now uses cryptographically random seeds generated at runtime, replacing the prior deterministic approach.
    • Salt encryption mechanism updated to derive from the generated seed with a fixed suffix.
    • RPC event field name standardized from "topic" to "topics".
    • Account generation now requires explicit seed values to proceed.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 21, 2025

Walkthrough

This change reorganizes seed generation responsibility in the account derivation flow. The system now generates a cryptographically secure random seed in the receive_address service before deriving the Starknet account, rather than relying on automatic seed generation within the client. The starknet client now requires explicit seeds, and voyager RPC event formatting corrects field naming from singular to plural "topics".

Changes

Cohort / File(s) Change Summary
Seed Generation Utility
utils/crypto/crypto.go
Added new exported function GenerateSecureSeed() that generates a 32-byte cryptographically secure random seed and returns it as a hex-encoded string, with corresponding encoding/hex import
Account Derivation Enforcement
services/starknet/client.go
Removed automatic seed generation logic from GenerateDeterministicAccount; now enforces non-empty seed requirement and removed crypto/rand and encoding/hex imports
Seed Generation Shift
services/receive_address.go
Updated to generate secure random seed at runtime using new crypto utility; modified salt encryption to use seed with "-paycrest" suffix instead of raw account salt bytes; added seed generation error handling
RPC Field Naming
services/voyager.go
Corrected RPC event field naming from "topic" to "topics" in both TransformVoyagerTransferToRPCFormat and TransformVoyagerEventToRPCFormat

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Key focus areas: Verify the new seed generation flow in receive_address.go properly integrates with account derivation and salt encryption; ensure removal of seed generation from client.go doesn't introduce regressions in existing callers; confirm RPC field naming corrections align with Starknet/Voyager specifications
  • Cross-file coordination: Review the handoff between receive_address.go (seed generation) and starknet/client.go (seed consumption) to ensure all callers provide required seeds

Suggested reviewers

  • chibie
  • 5ran6
  • onahprosper

Poem

🐰 A seed, once empty, now runs free—
Generated fresh for security!
From client's grip to address's care,
Randomness dances through the air. 🌱

Pre-merge checks and finishing touches

❌ Failed checks (5 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title claims to remove unused imports in starknet client, but the actual changes include significant refactoring of account generation logic, seed handling, and RPC event field updates unrelated to import cleanup. Update the title to accurately reflect the main changes: refactoring account generation to use secure random seeds and updating RPC event field names, not just import cleanup.
Description check ⚠️ Warning The PR description contains only the template with placeholder guidance text and no actual implementation details, leaving reviewers without understanding of the purpose, context, or impact of the changes. Fill in the Description section with details about the account generation refactoring, seed handling changes, and RPC field updates; provide references to linked issues; describe testing approach.
Linked Issues check ⚠️ Warning The PR changes (seed-based account generation, RPC field renames) do not address issue #407 requirements (saving previous provider rates in Redis priority queues for order assignment fallback). Either implement the priority queue rate-caching logic from issue #407 or remove the incorrect issue link and clarify which issue(s) this PR actually addresses.
Out of Scope Changes check ⚠️ Warning The PR includes multiple unrelated changes: seed-based account generation refactoring, RPC event field renames, and new crypto utility functions that appear disconnected from the stated purpose of removing unused imports. Clarify the scope of changes. Either consolidate related changes into one focused PR or update documentation to explain why these seemingly unrelated modifications are necessary together.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/update-receiveAddressServiceStarknet

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (2)
utils/crypto/crypto.go (1)

328-336: LGTM - Proper cryptographic seed generation.

The implementation correctly uses crypto/rand for secure randomness with appropriate entropy (256 bits). Minor nit: the comment says "generateSecureSeed" (lowercase) while the function is exported as GenerateSecureSeed.

🔎 Optional: Fix comment casing
-// generateSecureSeed generates a cryptographically secure random seed
+// GenerateSecureSeed generates a cryptographically secure random seed
 func GenerateSecureSeed() (string, error) {
services/receive_address.go (1)

54-71: Misleading variable naming: saltEncrypted stores the seed with suffix, not the salt.

The encrypted value stores seed + "-paycrest", which is the input used to derive the salt (via SHA256 in GenerateDeterministicAccount), not the salt itself. The actual salt is SHA256(seed + "-paycrest"). To reconstruct the account later, code must decrypt the value, strip the -paycrest suffix, and pass the raw seed back to GenerateDeterministicAccount (as done in services/order/starknet.go:95).

Consider renaming for clarity:

  • seedWithSuffixseedForStorage (more descriptive of purpose)
  • saltEncryptedseedEncrypted (accurately reflects contents)
  • Error message: update "failed to encrypt salt" to "failed to encrypt seed"

This eliminates the misleading naming and reduces the cognitive load for developers maintaining code that decrypts and uses this value.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d5bafc and b01a2d8.

📒 Files selected for processing (4)
  • services/receive_address.go (1 hunks)
  • services/starknet/client.go (1 hunks)
  • services/voyager.go (2 hunks)
  • utils/crypto/crypto.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/voyager.go (1)
utils/rpc_events.go (1)
  • TransferStarknetSelector (20-20)
services/receive_address.go (1)
utils/crypto/crypto.go (3)
  • GenerateSecureSeed (329-336)
  • EncryptPlain (40-62)
  • NormalizeStarknetAddress (313-325)
⏰ 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: build-and-test
🔇 Additional comments (3)
services/voyager.go (2)

1090-1090: LGTM - Field name aligns with RPC event format.

The rename from "topic" to "topics" is consistent with the event format used in services/starknet/client.go (see handleOrderCreated, handleOrderSettled, etc.), ensuring transformed Voyager events are compatible with the same processing logic.


1241-1241: Consistent with transfer event transformation.

Same field name correction applied here, maintaining consistency across both transformation functions.

services/starknet/client.go (1)

566-569: Good enforcement of explicit seed requirement.

Making the seed parameter mandatory with a clear error message is cleaner than implicit seed generation. Callers now use cryptoUtils.GenerateSecureSeed() explicitly, which improves traceability and testability.

@onahprosper onahprosper merged commit 054a3b8 into main Dec 21, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Save previous provider rates in priority queue to reduce order failures

3 participants