You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Overall this is a clean, well-structured addition. It follows the feature-guide profile (§3.1): the new recoveryAddress row summarizes and links to a dedicated ### recoveryAddress subsection under ## Refund Behavior, which is exactly the "keep the table cell concise, put the explanation in prose below" pattern from §2.7. Voice, callout usage (<Warning>/<Note>), and internal cross-links (root-relative anchors, lowercase #recoveryaddress) all match the guide. The #recoveryaddress anchor resolves correctly — Mintlify strips the backticks from the ### recoveryAddress`` heading and lowercases it.
A few minor notes and one clarification question:
1. Content clarification — does recoveryAddress require the auto-refund opt-in? (question, not a blocker)
The prose frames recoveryAddress as a fallback for "Relay's depositor detection" (features/deposit-addresses.mdx:948), and both the <Note> (line 937) and Recommended Setup (line 970) tell readers to pair it with the native-currency auto-refund opt-in. But the Constraints list (lines 962–964) only requires useDepositAddress: true and doesn't state whether refundTo must be set to the native-currency sentinel for recoveryAddress to have any effect. Is recoveryAddress meaningful when refundTo is a normal (non-sentinel) address, or is it only consulted when depositor auto-refund is active? Worth making explicit in the Constraints list so integrators know the required refundTo pairing.
2. refundTo table cell is getting long (minor, §2.7)
The auto-refund row at features/deposit-addresses.mdx:932 now carries three sentences (native-currency sentinel + VM support + CEX-detection abort behavior). §2.7 says to keep cell descriptions concise and move long explanations into prose below the table. The new CEX-detection sentence overlaps conceptually with the ### recoveryAddress section — consider trimming the cell to the sentinel/VM facts and letting the CEX-abort detail live in prose.
§2.9 recommends **`paramName`** (bold + backticks) for prose parameter references in guides. The new section uses backticks-only (`recoveryAddress`). This actually matches the local convention in deposit-addresses.mdx (e.g. refundTo at lines 46, 53 is backticks-only), so I'd leave it as-is for file consistency rather than introduce a mixed style — flagging only so it's a conscious choice.
Nothing here is blocking. Items 2 and 3 are stylistic; item 1 is the one worth a quick confirm from the author before merge so the constraints are unambiguous for integrators.
The changes since the last review are a clear improvement. The dedicated ### recoveryAddress section, the never receives refunds<Warning>, and the explicit Constraints list resolve most of the earlier ambiguity, and the refund-vs-recover distinction (withdraw from the depository contract) is now stated well. Structure follows the feature-guide profile (§3.1) and the concise-cell → prose-below pattern (§2.7). Anchors resolve correctly (### recoveryAddress`` → #recoveryaddress).
A few remaining points, none blocking:
1. refundTo pairing still not explicit in Constraints (carryover from prior review)
The prose consistently frames recoveryAddress as a fallback for "Relay's depositor detection" — i.e. the native-currency-sentinel auto-refund opt-in — and the bullet at features/deposit-addresses.mdx:952 ("Relay still attempts to auto-refund where possible") implies auto-refund must be active for recoveryAddress to matter. But the Constraints list (lines 962–964) still doesn't say whether refundTo must be set to the native-currency sentinel. An integrator reading only the Constraints could set recoveryAddress alongside a normal refundTo address and expect it to do something. Worth one explicit line: is recoveryAddress only consulted when the sentinel auto-refund is active, or is it independent of refundTo?
The <Note> at line 937 offers recoveryAddress to keep "depositor auto-detection for self-custodied senders", while the section at line 948 says it's "most relevant for deposits from custodial sources like centralized exchanges". Both are internally defensible (you opt into auto-detection expecting self-custodied senders; recoveryAddress catches the case where one turns out to be a CEX/blocked/unsupported depositor), but placing the two framings a few lines apart could read as contradictory. Consider a half-sentence tying them together.
The auto-refund row at line 932 now carries the sentinel + VM-support + CEX-detection-abort facts in one cell. §2.7 favors concise cells with detail in prose. The CEX-abort behavior overlaps with the new ### recoveryAddress section — trimming the cell and letting that detail live in prose is optional cleanup.
The section uses `recoveryAddress` rather than the guide's **`paramName`**, but this matches the local convention in deposit-addresses.mdx (e.g. refundTo), so keeping it as-is for file consistency is the right call.
Overall: ship-ready. Item 1 is the only one I'd suggest confirming before merge so the refundTo pairing is unambiguous for integrators.
• pedro/int2-769-communicate-recoveryto-and-zero-address-to-customers
The changes since the last review continue to sharpen this up — the dedicated ### recoveryAddress section, the "never receives refunds" <Warning>, and the explicit Constraints list read well and follow the feature-guide profile (§3.1) and the concise-cell → prose-below pattern (§2.7). Anchors resolve correctly (### recoveryAddress`` → #recoveryaddress; `## Refund Behavior` → `#refund-behavior`). Voice, callout usage, and root-relative links all match the guide.
One real internal-consistency issue and a couple of minor carryovers.
1. Contradiction: does auto-refund still run when recoveryAddress is supplied? (worth resolving)
Three passages disagree about what happens to refundTo behavior once recoveryAddress is set:
Table row (features/deposit-addresses.mdx:27): "existing refundTo behavior is preserved unlessrecoveryAddress is supplied" → implies supplying recoveryAddresschanges refund behavior.
Prose bullet (:954): "Existing refundTo behavior remains unchanged for flows that do not supplyrecoveryAddress" → again implies it changes when you do supply it.
Prose bullet (:952): "Relay still attempts to auto-refund where possible" → implies behavior is unchanged, and recoveryAddress only engages when auto-refund is impossible.
The <Warning> (:957) sides with the last reading — recoveryAddress "never receives refunds… only used to withdraw funds from the depository contract in the case that we cannot fill or refund." If recoveryAddress is purely additive (auto-refund still attempted; recovery is only a last resort), then the "preserved unless / unchanged for flows that do not supply" framings in lines 27 and 954 are misleading — they suggest a behavior change that the rest of the section says doesn't happen. Recommend rewording both to state plainly that recoveryAddress is additive and does not alter when or how refundTo refunds fire.
2. refundTo pairing still not stated in Constraints (carryover from prior reviews)
The prose frames recoveryAddress as a fallback for depositor auto-detection, but the Constraints list (:962–964) still only requires useDepositAddress: true. The <Warning> ("in the case that we cannot fill or refund") now implies recoveryAddress is consulted independently of the refundTo sentinel — which is a helpful signal — but it's never stated outright. One explicit Constraints line ("works regardless of refundTo value" / "requires the native-currency sentinel") would remove the last bit of ambiguity for an integrator reading only the list. Resolving item #1 above may settle this at the same time.
The auto-refund row at :932 now carries sentinel + VM-support + CEX-detection-abort in one cell. §2.7 favors concise cells with detail in prose, and the CEX-abort behavior overlaps with the new ### recoveryAddress section. Optional cleanup: trim the cell to the sentinel/VM facts and let the CEX-abort detail live in prose.
4. Minor: "EOA" asserted in prose but not in Constraints (very minor)
The table (:27) and section intro (:946) specify an EOA, but the Constraints bullet (:963) only says "a valid address on the origin chain… cannot be the native-currency address" — it doesn't restate the EOA requirement. If EOA is a hard constraint (needed to sign the depository withdrawal), consider adding it to the list so the constraint is discoverable there too.
Overall: ship-ready. Item 1 is the only one I'd resolve before merge — it's a genuine reader-facing contradiction, not just style.
• pedro/int2-769-communicate-recoveryto-and-zero-address-to-customers
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
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.
No description provided.