Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diagnostic data: sanitize swap and reverse swap infos #1063

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Aug 6, 2024

This PR ensures our diagnostic data doesn't include sensitive fields for swaps and reverse swaps.

libs/sdk-core/src/models.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/models.rs Outdated Show resolved Hide resolved
@ok300
Copy link
Contributor Author

ok300 commented Aug 9, 2024

@JssDWt @roeierez I simplified the PR based on the feedback above: c9dc874

It now:

  • allows for both sanitized serialization (used in generate_sdk_diagnostic_data) and preserves the default non-sanitized serialization (used elsewhere, like when reconstructing SwapInfos as part of list_payments)
  • defining the sanitization logic doesn't need to be updated when new fields are added to the underlying struct (only the sanitized fields have to be explicitly stated)
  • defining the sanitization logic is simple to do for new structs (just implement Sanitize<T>)

What do you think?

@ok300 ok300 requested review from JssDWt and roeierez August 9, 2024 09:24
Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, otherwise LGTM

libs/sdk-core/src/models.rs Outdated Show resolved Hide resolved
Co-authored-by: Jesse de Wit <witdejesse@hotmail.com>
@ok300 ok300 merged commit 8271114 into main Aug 15, 2024
9 checks passed
@ok300 ok300 deleted the ok300-sanitize-diagnostic-data branch August 15, 2024 13:23
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.

3 participants