fix(logging): record metadata values that were silently dropped - #1436
Open
bmc08gt wants to merge 2 commits into
Open
fix(logging): record metadata values that were silently dropped#1436bmc08gt wants to merge 2 commits into
bmc08gt wants to merge 2 commits into
Conversation
MetadataBuilder declared `infix fun String.to(value: Any)`. A non-null parameter makes the member inapplicable to a nullable argument, so the call resolved to kotlin.to instead, built a Pair and discarded it in statement position. The field never reached the log. It compiles clean and produces at most an unused-expression warning, so nothing flagged it. The declared type is what breaks it, not the runtime value: a String? holding "hello" was dropped just as silently as one holding null. An overload taking Any? alongside the existing Any is not possible — both erase to to(String, Object)V and Kotlin rejects the platform declaration clash. Widen the single member to Any? instead, recording NULL_PLACEHOLDER for a null. That keeps it the only applicable candidate, so kotlin.to can no longer win. Audited the repo by temporarily shadowing kotlin.to inside the builder with a @deprecated candidate and compiling every module. Eight sites were dropping fields: NotificationService title, body CoinbaseOnRampController correlationId, responseBody, errorLink GooglePlayBillingClient purchases SwapViewModel errorMessage RealVerifiedFiatCalculator balance quarks Each is now explicit at the call site rather than leaning on the placeholder, so an absent value reads as what the field means: -1 for a count, "none" for an amount, "" where the file already used orEmpty. Push content stays out — TraceType.Process reaches breadcrumb sinks, so title and body become the derived `silent` and `has_body`. MetadataBuilderTest covers both nullable cases. Five of its seven tests fail if the parameter is narrowed back to Any.
GiftCardFundingWorker recorded `giftCard.entropy` in both funding traces. That value is mnemonic.getBase58EncodedEntropy(), the seed controlling the gift card's funds, and trace() with any type other than Silent forwards its metadata to breadcrumb sinks. It was reaching Bugsnag on every funding success and failure. The vault public key identifies the same account for debugging and is already public on-chain. The WorkManager tag built in tagFor still embeds entropy. That stays in the WorkManager database rather than going to a breadcrumb sink, so it is left alone here.
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
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.
MetadataBuilderdeclaredinfix fun String.to(value: Any). A non-null parameter makes the member inapplicable to a nullable argument, so the call resolves tokotlin.toinstead, builds aPairand discards it in statement position. The field never reaches the log. It compiles clean and produces at most an unused-expression warning, so nothing flagged it.The declared type is what breaks it, not the runtime value. A
String?holding"hello"was dropped just as silently as one holdingnull.The fix
Adding an
Any?overload alongside the existingAnyis not possible — both erase toto(String, Object)V, and Kotlin rejects the platform declaration clash. The single member is widened toAny?instead, recordingNULL_PLACEHOLDERfor a null. That keeps it the only applicable candidate, sokotlin.tocan no longer win for anyStringkey.Audit
kotlin.towas temporarily shadowed inside the builder with a@Deprecatedcandidate, which makes exactly the fallback call sites diagnose while leaving correct ones silent. Compiling every module's main and unit-test sources found eight sites dropping fields:NotificationServicetitle,bodyCoinbaseOnRampControllercorrelationId,responseBody,errorLinkGooglePlayBillingClientpurchasesSwapViewModelerrorMessageRealVerifiedFiatCalculatorbalance quarksEach is now explicit at the call site rather than leaning on the placeholder, so an absent value reads as what the field means:
-1for a count,"none"for an amount,""where the file already usedorEmpty.What is not recorded
These traces reach breadcrumb sinks for every
TraceTypeexceptSilent, so unmasking a dropped field can start sending content to Bugsnag that was previously discarded by accident.Push content stays out:
titleandbodybecome the derivedsilentandhas_body. The CoinbaseresponseBodyis kept, matching the sibling path in the same file — the error envelope is a fixed schema ofcorrelationId,errorType,errorLink,codeandmessage, and it is most useful exactly whenparse()returns null.The second commit is a separate leak the audit surfaced rather than a nullability bug.
GiftCardFundingWorkerrecordedgiftCard.entropy—mnemonic.getBase58EncodedEntropy(), the seed controlling the gift card's funds — in both funding traces. It is non-null, so it was reaching Bugsnag on every success and failure. It now logs the vault public key, which identifies the same account and is already public on-chain.Tests
MetadataBuilderTestcovers aString?holding a value and one holding null, plus nullable non-String types. Five of its seven tests fail if the parameter is narrowed back toAny.