data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt (2)

668-673: Named constant LGTM.

ERR_TX_IN_MEMPOOL_CACHE = 19 matches the Cosmos SDK ErrTxInMempoolCache error code and removes the magic number at the call site. Consider documenting the upstream reference in a comment, but not blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 668 - 673, Add a short comment above the companion object constant
ERR_TX_IN_MEMPOOL_CACHE explaining it maps to the Cosmos SDK ErrTxInMempoolCache
(value 19) and, optionally, include a reference or link to the upstream source
(e.g., Cosmos SDK error definition) so future readers know the origin of the
value; update the comment near the ERR_TX_IN_MEMPOOL_CACHE constant in the
companion object of ThorChainApi to document this mapping.

---

292-313: Broadcast refactor looks good; one heads-up on the catch-and-rethrow log.

Switching to bodyAsText() + json.decodeFromString cleanly removes the double body read, and the named ERR_TX_IN_MEMPOOL_CACHE constant reads much better than the bare 19.

The smart-cast on line 306 (txResponse.txHash after txResponse?.code == 0 || txResponse?.code == ERR_TX_IN_MEMPOOL_CACHE) is fine — Kotlin propagates non-nullness through the comparisons.

Note (out of scope for this PR, just flagging): the existing line 310 Timber.tag("THORChainService").e("Error broadcasting transaction: ${e.message}") swallows the exception object (no stack trace) and uses a string template. If you want to align with the new logging style adopted elsewhere in this PR, Timber.e(e, "Error broadcasting transaction") would be consistent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 292 - 313, The catch block in broadcastTransaction currently logs only
e.message via a string template and loses the stack trace; replace the Timber
call so the throwable is passed to Timber's overload that records the exception
(e.g., use the same tag "THORChainService" but call the Timber e(...) overload
that accepts the Throwable and a message), i.e. change the
Timber.tag("THORChainService").e("Error broadcasting transaction: ${e.message}")
call to the form that passes e as the first argument and a static message "Error
broadcasting transaction" so the stack trace is preserved while keeping context;
leave the rest of broadcastTransaction and the ERR_TX_IN_MEMPOOL_CACHE logic
unchanged.

data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/MidgardNetworkData.kt (1)

6-10: Redundant @SerialName annotations.

Both @SerialName values match the Kotlin property names exactly, so the annotations are no-ops and can be dropped to reduce noise. Keep them only if you anticipate renaming the Kotlin properties while preserving the wire format.

♻️ Proposed simplification

 `@Serializable`
 data class MidgardNetworkData(
-    `@SerialName`("bondingAPY") val bondingAPY: String,
-    `@SerialName`("nextChurnHeight") val nextChurnHeight: String,
+    val bondingAPY: String,
+    val nextChurnHeight: String,
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/MidgardNetworkData.kt`
around lines 6 - 10, The `@SerialName` annotations in the MidgardNetworkData data
class are redundant because their values match the Kotlin property names; remove
the `@SerialName`(...) wrappers from the properties bondingAPY and nextChurnHeight
in MidgardNetworkData and leave the `@Serializable` annotation intact, and if the
kotlin.serialization.SerialName import becomes unused after removal, delete that
import as well.

app/src/main/java/com/vultisig/wallet/ui/screens/send/VerifySendScreen.kt (1)

49-49: Cross‑module import of a UI sentinel is a smell.

VerifySendScreen (under ui/screens/send) reaching into ui/models/mappers to import a display string and use it as a control‑flow sentinel signals that the abstraction boundary is wrong. See the corresponding comments on TokenValueToDecimalUiStringMapper.kt and on lines 197–211 — the cleanest fix is to expose a typed isUnlimited flag on the UI model so this import (and the string comparison) can go away entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/com/vultisig/wallet/ui/screens/send/VerifySendScreen.kt` at
line 49, VerifySendScreen is importing the UI sentinel UNLIMITED_TOKEN_AMOUNT
from ui/models/mappers and using string comparison as control flow; replace this
cross‑module string check by adding a typed boolean on the token UI model (e.g.,
add isUnlimited to the model produced by TokenValueToDecimalUiStringMapper/its
output type) and update VerifySendScreen to use model.isUnlimited instead of
comparing against UNLIMITED_TOKEN_AMOUNT so the import and string comparison can
be removed.

app/src/main/java/com/vultisig/wallet/ui/models/mappers/TokenValueToDecimalUiStringMapper.kt (2)

21-22: Sentinel-string coupling between mapper and screen — prefer a structured signal.

Returning "Unlimited" from the mapper and then having VerifySendScreen string‑match against the same constant to decide whether to render a warning icon couples UI rendering decisions to the formatter's output. This is fragile:

• Any future change to the display string (capitalization, localization, punctuation, etc.) silently breaks the warning.
• The mapper now has a UI‑policy concern ("which token values deserve a warning") that's outside its responsibility (decimal formatting).

Consider moving the "is this an unlimited/max approval" decision onto the data model or VM (e.g., a Boolean isUnlimited flag on TransactionDetailsUiModel.token / ValuedToken, computed once where the raw TokenValue is available). The mapper can then keep formatting concerns; the screen reads the boolean. Equality against MAX_UINT256 also becomes more discoverable than buried inside a formatter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/com/vultisig/wallet/ui/models/mappers/TokenValueToDecimalUiStringMapper.kt`
around lines 21 - 22, The mapper TokenValueToDecimalUiStringMapper.invoke
currently returns the sentinel string UNLIMITED_TOKEN_AMOUNT when from.value ==
MAX_UINT256 which forces VerifySendScreen to string-match that value; instead,
compute and expose a Boolean (e.g., isUnlimited) on the UI model
(TransactionDetailsUiModel.token or ValuedToken) where the raw TokenValue is
available (set isUnlimited = from.value == MAX_UINT256 in the VM/mapper layer),
change TokenValueToDecimalUiStringMapper.invoke to always return the formatted
decimal string (no sentinel), and update VerifySendScreen to read the new
isUnlimited flag to decide rendering of the warning icon rather than comparing
against UNLIMITED_TOKEN_AMOUNT.

---

21-42: MAX_UINT256 short-circuits before the try, so a malformed TokenValue with that exact raw value still bypasses the safety net.

Minor: the new check executes outside the try/catch. In practice BigInteger == BigInteger won't throw, so this is fine today, but if the equality check is later replaced with anything that can throw (e.g., from.decimal based detection), the catch‑all fallback to BigDecimal.ZERO will be skipped. Worth keeping in mind if this branch grows.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/com/vultisig/wallet/ui/models/mappers/TokenValueToDecimalUiStringMapper.kt`
around lines 21 - 42, Move the MAX_UINT256 short-circuit into the existing try
block so the equality check is executed inside the try/catch in
invoke(TokenValue), e.g., start the try before checking if (from.value ==
MAX_UINT256) and return UNLIMITED_TOKEN_AMOUNT there; this ensures any future
changes to the detection logic (or exceptions thrown by TokenValue) are caught
by the catch that logs via Timber and returns BigDecimal.ZERO.

data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt (5)

535-535: Replace string-template in Timber call with format specifier.

♻️ Suggested fix

-            Timber.e(e, "Failed to fetch denom metadata for $denom")
+            Timber.e(e, "Failed to fetch denom metadata for %s", denom)

As per coding guidelines: "Use Timber format specifiers (%s, %d) instead of string templates in log calls".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` at line
535, Replace the string-template usage in the Timber error call with a format
specifier: locate the Timber.e(...) call in ThorChainApi (the one logging
"Failed to fetch denom metadata for $denom") and change it to use a format
placeholder (e.g., "%s") and pass denom as an argument so the call becomes
Timber.e(e, "Failed to fetch denom metadata for %s", denom).

---

156-187: Remaining hardcoded URLs and body<T>() calls — follow-up cleanup.

Per the PR objectives ("Remove hardcoded Thornode URLs in favor of companion-object constants" and "make consistent use of bodyOrThrow<T>()"), getUnstakableTcyAmount (line 159) and getTcyAutoCompoundAmount (line 174) still hardcode https://thornode.thorchain.network/... and use raw response.body<TcyStakerResponse>() / response.body<ThorTcyBalancesResponseJson>(). The companion already exposes THORNODE_BASE. Same pattern applies to several other endpoints in this file (e.g., getBalance, getAccountNumber, getTHORChainNativeTransactionFee, getTHORChainReferralFees, getNetworkChainId, resolveName, getTransactionDetail).

Since the PR description explicitly says not to perform large unrelated refactors, this is acceptable to defer — but please track it as a follow-up so the codebase converges on the documented convention.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 156 - 187, getUnstakableTcyAmount and getTcyAutoCompoundAmount still use
hardcoded Thornode URLs and direct response.body<T>(); change their URL
construction to use the companion constant THORNODE_BASE (e.g.,
"$THORNODE_BASE/thorchain/tcy_staker/$address" and
"$THORNODE_BASE/cosmos/bank/v1beta1/balances/$address") and replace
response.body<TcyStakerResponse>() /
response.body<ThorTcyBalancesResponseJson>() with the standard
response.bodyOrThrow<T>() helper so errors are propagated; apply the same
pattern to other methods mentioned (getBalance, getAccountNumber,
getTHORChainNativeTransactionFee, getTHORChainReferralFees, getNetworkChainId,
resolveName, getTransactionDetail) and file these as follow-up cleanup tasks if
not addressed in this PR.

---

612-613: Use Timber format args and include status in error(...) message.

Two nits in this branch:

• The format-arg passes a string-templated value ("${httpResponse.status}") instead of letting Timber stringify it.
• The thrown error message "Error Fetching Tcy Staked: " has a dangling colon and no useful context.

♻️ Suggested fix

-            Timber.e("FetchTcyStakedAmount %s", "${httpResponse.status}")
-            error("Error Fetching Tcy Staked: ")
+            Timber.e("FetchTcyStakedAmount %s", httpResponse.status)
+            error("Error Fetching Tcy Staked: ${httpResponse.status}")

As per coding guidelines: "Use Timber format specifiers (%s, %d) instead of string templates in log calls".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 612 - 613, In ThorChainApi.kt replace the Timber call to use a format arg
(e.g. Timber.e("FetchTcyStakedAmount %s", httpResponse.status)) instead of
passing a string-templated status, and update the thrown error to include the
HTTP status (e.g. error("Error fetching Tcy staked:
status=%s".format(httpResponse.status)) or build the message with the status) so
the error message is not dangling and contains useful context; locate the two
statements around the fetchTcyStakedAmount handling (the Timber.e and the
error(...) call) and change them accordingly.

---

286-290: Prefer error(...) over raw Exception and avoid double-logging the response body.

Two small things:

1. As per coding guidelines ("Use require/check/error for preconditions instead of raw exceptions"), throwing a raw Exception is discouraged; use error(...) (which throws IllegalStateException) or a domain-specific exception type.
2. The thrown message already contains responseRawString, and the catch then re-logs via Timber...e(e, "Error broadcasting transaction") — so the body is captured once via the exception's message. That's fine, but logging plus rethrow means the caller will likely log the same exception again. Consider either logging here and swallowing, or rethrowing without the local log to avoid duplicate entries upstream.

♻️ Suggested fix

-            throw Exception("Error broadcasting transaction: $responseRawString")
+            error("Error broadcasting transaction: $responseRawString")

As per coding guidelines: "Use require/check/error for preconditions instead of raw exceptions".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 286 - 290, Replace the raw throw Exception(...) with error("Error
broadcasting transaction: $responseRawString") (which throws
IllegalStateException) and remove the local catch/logging to avoid
double-logging; specifically, in ThorChainApi.kt update the broadcasting code
that currently throws Exception and calls Timber.tag("THORChainService").e(...)
to instead use error(...) and eliminate the catch block (or at minimum stop
calling Timber.e before rethrowing) so the exception message contains the
response body without redundant local logging.

---

282-285: Adopt explicit null-check for clarity.

Kotlin 2.1.20 (K2) does support smart-cast through txResponse?.code == 0, so the current code is syntactically valid. However, Kotlin guidelines recommend explicit null handling (?.let, ?:) for readability. The defensive pattern clarifies intent and aligns with the project's null-safety practices:

-            val txResponse = result.txResponse
-            if (txResponse?.code == 0 || txResponse?.code == ERR_TX_IN_MEMPOOL_CACHE) {
-                return txResponse.txHash
-            }
+            val txResponse = result.txResponse
+                ?: error("Missing tx_response in broadcast result")
+            if (txResponse.code == 0 || txResponse.code == ERR_TX_IN_MEMPOOL_CACHE) {
+                return txResponse.txHash
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/main/kotlin/com/vultisig/wallet/data/api/ThorChainApi.kt` around
lines 282 - 285, The txResponse null-safety check in ThorChainApi should be made
explicit: instead of relying on the nullable smart-cast in the conditional,
first guard or unwrap result.txResponse (e.g., using txResponse?.let { } or an
early if (txResponse == null) return) and then inspect its code against 0 and
ERR_TX_IN_MEMPOOL_CACHE; update the block that currently references
txResponse?.code so txResponse is non-null when accessing txHash to improve
readability and follow the project's null-handling convention.

data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelper.kt (1)

5-41: Behavior preserved cleanly; consider documenting the 50 bps short-circuit.

The discount tiering is correct (50+ bps → 0 bps affiliate, otherwise clamp via calculateBpsAfterDiscount), and extracting this out of ThorChainApiImpl is a good cohesion win. The only nit: the discountBps >= 50 branch has no comment explaining why a discount at/above 50 bps zeroes the affiliate address rate while still keeping the address — a brief KDoc above the function describing this contract (and what the upper bound 50 represents) would help future readers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelper.kt`
around lines 5 - 41, Add a KDoc comment above buildAffiliateParams explaining
the 50 bps short-circuit: state that when discountBps >= 50 the function returns
the AFFILIATE_FEE_ADDRESS with affiliate_bps set to "0" (preserving the address
but zeroing the rate), document what the 50 value represents (the maximum
discount tier that nullifies affiliate fees), and mention interactions with
calculateBpsAfterDiscount and constants like
THORChainSwaps.AFFILIATE_FEE_ADDRESS and REFERRED_AFFILIATE_FEE_RATE_BP so
future readers understand the contract and why the branch exists.

data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/ThorChainTransactionJson.kt (1)

6-11: @SerialName("rawLog") doesn't match the Cosmos SDK wire format.

tx_response.raw_log in Cosmos REST is snake_case. The current @SerialName mappings here (code, codespace, rawLog) are mixed: two snake/lowercase, one camelCase. Since this DTO is now populated programmatically from CosmosEnvelopedTxResponse (deserialization happens there), the @Serializable/@SerialName annotations are arguably unused — but if anything ever deserializes this class directly (e.g., a cached JSON, a future endpoint, or a test fixture), rawLog will silently fail to map.

Either drop the annotations or align with the wire format:

♻️ Suggested fix

 `@Serializable`
 data class ThorChainTransactionJson(
     `@SerialName`("code") val code: Int?,
     `@SerialName`("codespace") val codeSpace: String?,
-    `@SerialName`("rawLog") val rawLog: String,
+    `@SerialName`("raw_log") val rawLog: String,
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/ThorChainTransactionJson.kt`
around lines 6 - 11, The `@SerialName` for rawLog is mismatched to Cosmos wire
format; update the ThorChainTransactionJson DTO so its serialized names match
the REST fields (or remove the annotations entirely). Specifically, either
remove all `@SerialName` annotations from data class ThorChainTransactionJson
(letting default Kotlin names be used) or change `@SerialName`("rawLog") to
`@SerialName`("raw_log") and ensure the codeSpace field uses
`@SerialName`("codespace") and code uses `@SerialName`("code") to align with the
Cosmos tx_response fields (code, codespace, raw_log).

data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/RujiGraphQLResponse.kt (1)

22-27: Tighten nullability based on GraphQL schema.

StakingV2.bonded and StakingV2.account are declared non-nullable, but stakingV2 is List<StakingV2?>? (the inner element is nullable). If the GraphQL server can return a null element (or if any non-nullable field comes back missing), decodeFromString will throw at the call site in getRujiStakeBalance, which currently has no catch. Either confirm both bonded and account are guaranteed by the schema and drop the inner nullable on List<StakingV2?>?, or make these fields nullable to fail soft.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/api/models/thorchain/RujiGraphQLResponse.kt`
around lines 22 - 27, The StakingV2 model currently declares account and bonded
as non-nullable while the response type used elsewhere is List<StakingV2?>?
which allows null elements; update the model or the response typing to be
consistent: either (A) if GraphQL guarantees account and bonded, change the
response type to List<StakingV2>? (drop the inner nullable) where
getRujiStakeBalance decodes results, or (B) make StakingV2.account and
StakingV2.bonded nullable so decoding can tolerate missing fields and the caller
in getRujiStakeBalance can handle nulls safely; pick one approach and change the
StakingV2 data class and/or the response list type accordingly so
decodeFromString no longer crashes on nullable elements.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt (2)

27-33: Consider one missing case: null/empty tx_response envelope.

broadcastTransaction guards result.txResponse ?: error(...) (Line 282-283 of ThorChainApi.kt), and getTransactionDetail may need the same guard depending on the CosmosEnvelopedTxResponse.txResponse nullability (see the separate comment on Line 329-334 of ThorChainApi.kt). A test feeding {} or {"tx_response": null} to either method would lock in the "structured error, not NPE" contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt`
around lines 27 - 33, Add tests in ThorChainApiImplTest that exercise the
null/empty tx_response envelope path to ensure structured errors (not NPE) are
returned: use newApi(...) with MockHttpClient.respondingWith(HttpStatusCode.OK,
"{}") and with "{\"tx_response\": null}" and call
ThorChainApiImpl.broadcastTransaction(...) and
ThorChainApiImpl.getTransactionDetail(...), asserting they surface the expected
error/exception class or message rather than throwing NullPointerException;
reference the broadcastTransaction and getTransactionDetail methods and the
CosmosEnvelopedTxResponse txResponse field to locate where the guard is needed
and to lock in the contract.

---

7-7: Optional: prefer runTest over runBlocking for suspend-test bodies.

runBlocking parks the calling thread; kotlinx.coroutines.test.runTest is the modern idiom for suspending tests, gives you a TestScope, virtual-time advancement, and faster execution. The existing MockHttpClient likely doesn't need scheduler control here, but using runTest future-proofs these tests against any added delay/withTimeout paths and aligns with the project's stated "avoid runBlocking" preference.

As per coding guidelines: "Avoid runBlocking — use suspend functions and proper coroutines with viewModelScope".

Also applies to: 35-37, 58-59, 80-81, 91-97, 100-101, 119-120, 138-139

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt` at
line 7, Replace usages of kotlinx.coroutines.runBlocking in
ThorChainApiImplTest.kt with kotlinx.coroutines.test.runTest: add the import for
runTest and change each test body that currently uses runBlocking (including the
top-level import and the test functions referenced) to use runTest so tests run
in a TestScope with virtual-time support; update any test functions (involving
the ThorChainApiImplTest test methods) to be suspend lambdas compatible with
runTest.

data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelper.kt (2)

15-15: Nit: tighten the short-circuit comment.

The comment reads a little awkwardly. Consider rewording to make the intent obvious — e.g. // Full discount: drop all affiliate fees and ignore the referral code. This also documents why the referral code is intentionally ignored on this branch (which the test on Line 80-87 of ThorChainAffiliateHelperTest.kt exercises).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelper.kt`
at line 15, Update the terse comment inside ThorChainAffiliateHelper (same
file/class) to clearly state the branch's intent; replace the current line "//
For ultimate clean referral, and return 0 bps affiliate" with a clearer phrase
such as "// Full discount: drop all affiliate fees and ignore the referral
code." so readers and the ThorChainAffiliateHelperTest.kt case (the test that
exercises the full-discount branch) immediately understand why the referral code
is intentionally ignored.

---

13-48: Optional: simplify with buildMap/mapOf for clearer intent.

The mutableMapOf + branch-mutate pattern works correctly, but each branch ends up populating a fixed pair of keys. A buildMap/mapOf per branch makes it obvious that the result is always exactly {affiliate, affiliate_bps} (which is what the contract test on Line 99-110 of ThorChainAffiliateHelperTest.kt asserts) and removes the need for the mutableMapOf and explicit returns.

♻️ Proposed refactor

-    fun buildAffiliateParams(referralCode: String, discountBps: Int): Map<String, String> {
-        val affiliateParams = mutableMapOf<String, String>()
-
-        // For ultimate clean referral, and return 0 bps affiliate
-        if (discountBps >= 50) {
-            affiliateParams["affiliate"] = THORChainSwaps.AFFILIATE_FEE_ADDRESS
-            affiliateParams["affiliate_bps"] = "0"
-
-            return affiliateParams
-        }
-
-        if (referralCode.isNotEmpty()) {
-            val affiliateFeeRateBp =
-                calculateBpsAfterDiscount(
-                    baseBps = THORChainSwaps.REFERRED_AFFILIATE_FEE_RATE_BP,
-                    discountBps = discountBps,
-                )
-
-            // Build nested affiliate with new thorchain structure
-            val affiliates = "$referralCode/${THORChainSwaps.AFFILIATE_FEE_ADDRESS}"
-            val affiliateBps = "${THORChainSwaps.REFERRED_USER_FEE_RATE_BP}/$affiliateFeeRateBp"
-
-            affiliateParams["affiliate"] = affiliates
-            affiliateParams["affiliate_bps"] = affiliateBps
-        } else {
-            val affiliateFeeRateBp =
-                calculateBpsAfterDiscount(
-                    baseBps = THORChainSwaps.AFFILIATE_FEE_RATE_BP,
-                    discountBps = discountBps,
-                )
-
-            affiliateParams["affiliate"] = THORChainSwaps.AFFILIATE_FEE_ADDRESS
-            affiliateParams["affiliate_bps"] = affiliateFeeRateBp.toString()
-        }
-
-        return affiliateParams
-    }
+    fun buildAffiliateParams(referralCode: String, discountBps: Int): Map<String, String> {
+        // Ultimate clean referral: zero affiliate fee.
+        if (discountBps >= 50) {
+            return mapOf(
+                "affiliate" to THORChainSwaps.AFFILIATE_FEE_ADDRESS,
+                "affiliate_bps" to "0",
+            )
+        }
+
+        return if (referralCode.isNotEmpty()) {
+            val affiliateFeeRateBp = calculateBpsAfterDiscount(
+                baseBps = THORChainSwaps.REFERRED_AFFILIATE_FEE_RATE_BP,
+                discountBps = discountBps,
+            )
+            mapOf(
+                "affiliate" to "$referralCode/${THORChainSwaps.AFFILIATE_FEE_ADDRESS}",
+                "affiliate_bps" to "${THORChainSwaps.REFERRED_USER_FEE_RATE_BP}/$affiliateFeeRateBp",
+            )
+        } else {
+            val affiliateFeeRateBp = calculateBpsAfterDiscount(
+                baseBps = THORChainSwaps.AFFILIATE_FEE_RATE_BP,
+                discountBps = discountBps,
+            )
+            mapOf(
+                "affiliate" to THORChainSwaps.AFFILIATE_FEE_ADDRESS,
+                "affiliate_bps" to affiliateFeeRateBp.toString(),
+            )
+        }
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelper.kt`
around lines 13 - 48, The function currently constructs affiliateParams with
mutableMapOf and mutates it in branches; instead simplify by returning an
immutable map per branch (use buildMap or mapOf) so each path explicitly returns
a Map containing exactly the "affiliate" and "affiliate_bps" keys; update the
logic in the method that builds affiliateParams (the branch handling discountBps
>= 50, the referralCode.isNotEmpty() branch, and the else branch where
affiliateFeeRateBp is computed) to return a map literal from each branch rather
than mutating a shared mutableMapOf and using early returns.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

data/src/test/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelperTest.kt (1)

100-109: Optional: consider @ParameterizedTest for the case loop.

The for loop on Lines 102–109 effectively runs multiple sub-cases under a single @Test. Migrating to @ParameterizedTest with @MethodSource/@CsvSource would surface each case as an independent test result (clearer failure attribution in CI), without changing coverage. Non-blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@data/src/test/kotlin/com/vultisig/wallet/data/chains/helpers/ThorChainAffiliateHelperTest.kt`
around lines 100 - 109, Convert the looped assertions into a parameterized test
so each case reports separately: replace the current for-loop inside
ThorChainAffiliateHelperTest with a `@ParameterizedTest` (e.g., using `@CsvSource`
or `@MethodSource`) that feeds (referralCode, discountBps) pairs into a test
method which calls ThorChainAffiliateHelper.buildAffiliateParams and asserts the
keys equal setOf("affiliate", "affiliate_bps"); ensure the new test method
signature accepts the two parameters and preserves the existing assertion
message (or derive case description from the parameters) so CI shows individual
failing cases.

data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt (2)

28-34: Consider relaxed = true on the swap-quote serializer mock for safety.

mockk<ThorChainSwapQuoteResponseJsonSerializer>() is a strict mock with no stubs. None of the current tests exercise swap-quote paths so this is fine today, but if a future test in this class accidentally hits a code path that uses the serializer, the failure will be a MockKException rather than something obvious. A relaxed mock (or passing relaxed = true) would degrade more gracefully.

-            thorChainSwapQuoteResponseJsonSerializer =
-                mockk<ThorChainSwapQuoteResponseJsonSerializer>(),
+            thorChainSwapQuoteResponseJsonSerializer =
+                mockk<ThorChainSwapQuoteResponseJsonSerializer>(relaxed = true),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt`
around lines 28 - 34, The mock for ThorChainSwapQuoteResponseJsonSerializer used
when constructing ThorChainApi in newApi is strict and can throw MockKException
if later tests hit serializer behavior; change the mock creation to a relaxed
mock (use mockk<ThorChainSwapQuoteResponseJsonSerializer>(relaxed = true)) when
instantiating ThorChainApiImpl so the serializer degrades gracefully without
explicit stubs; update the mock passed to ThorChainApiImpl in the newApi helper
to use relaxed = true.

---

59-79: Test name vs. assertions: "surfaces" suggests throwing — clarify intent.

The name surfaces non-zero tx_response code and codespace reads as if a non-zero code should be propagated as an error, but the body just verifies the fields are mapped onto the DTO (which is the correct behavior per the PR — getTransactionDetail does not throw for on-chain failures, only broadcastTransaction does). Consider renaming for clarity, e.g. getTransactionDetail maps non-zero tx_response code, codespace, and raw_log to DTO. This also makes the contrast with broadcastTransaction throws on non-zero non-mempool-cache code (line 140) explicit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@data/src/test/kotlin/com/vultisig/wallet/data/api/ThorChainApiImplTest.kt`
around lines 59 - 79, Rename the test function to clarify it checks mapping
rather than error propagation: change the test name `getTransactionDetail
surfaces non-zero tx_response code and codespace` to something like
`getTransactionDetail maps non-zero tx_response code, codespace, and raw_log to
DTO` so it accurately reflects that ThorChainApiImpl.getTransactionDetail maps
the tx_response fields (code, codespace, raw_log) onto the DTO rather than
throwing (contrast to broadcastTransaction behavior); update the test function
identifier and any references to the old name accordingly.