Conversation
…faces - Define MailboxConfig with ISM and Hook artifact references - Define ValidatorAnnounceConfig with mailbox address reference - Add raw artifact variants for protocol implementation use - Export new modules in package.json - Follow existing ISM/Hook artifact patterns
- RadixMailboxArtifactManager implements IRawMailboxArtifactManager - RadixMailboxReader reads mailbox config from chain (owner, ISM, hooks) - RadixMailboxWriter supports create and update operations - Create deploys mailbox and sets ISM/hooks via setters - Update compares state and generates update transactions - Exports RadixMailboxArtifactManager from index
- Test create mailbox with ISM and hooks - Test read mailbox configuration from chain - Test update mailbox when ISM changes - Test update mailbox when owner changes - Test no-op updates when state matches - Test readMailbox convenience method
- Created validator-announce.ts combining query and tx functions - RadixValidatorAnnounceArtifactManager implements IRawValidatorAnnounceArtifactManager - RadixValidatorAnnounceReader reads mailbox address from chain - RadixValidatorAnnounceWriter creates validator announce (immutable, no updates) - Added comprehensive e2e tests for create, read, update (no-op), and convenience methods - Exported RadixValidatorAnnounceArtifactManager from index
- Create test mailbox in before() hook instead of using burn address - Validator announce requires valid mailbox contract address - Fixes NoMethodMapping error when trying to call local_domain on Account - All 5 validator announce e2e tests now passing
…reading - Implements ArtifactReader<MailboxConfig, DeployedMailboxAddress> - Orchestrates raw mailbox reader + ISM/hook readers to expand nested artifacts - Provides deriveCoreConfig() for backward compatibility with AltVMCoreReader - Moves mailboxArtifactToDerivedCoreConfig to provider-sdk/src/mailbox.ts - Exports ismArtifactToDerivedConfig from deploy-sdk ISM reader - Cleans up provider-sdk/src/core.ts to keep only legacy types
…h zero addresses - Add ConfigOnChain<C> type to preserve nested artifact deployment types - Rename RawMailboxConfig to MailboxOnChain for clarity - Add getOptionalFieldValueFromEntityState helper for Option::None handling - Skip setter transactions when zero addresses provided (optimization) - Update RadixMailboxWriter to conditionally set ISM/hooks - Add test coverage for zero address mailbox creation - Normalize optional fields to zero address in RadixProvider - Remove 'any' types from mailboxArtifactToDerivedCoreConfig converters
…ition from deploy-sdk after rebase
xeno097
changed the title
…to andrey/mailbox-validator-artifact-interfaces
…to andrey/mailbox-validator-artifact-interfaces
…to andrey/mailbox-validator-artifact-interfaces
…to andrey/mailbox-validator-artifact-interfaces
|
@claude brutally honest review |
|
Claude finished @xeno097's task in 3m 14s —— View job Brutally Honest ReviewAlright, you asked for it. Overall code quality is solid, but there's unnecessary complexity creeping in. 🟢 The GoodType safety: No Test coverage: Comprehensive e2e tests for both mailbox and validator announce. Tests cover create, read, update, and edge cases like zero addresses. Error handling: Proper use of Backward compatibility: The Changesets: Well-written, past tense, descriptive. Follow the guidelines. 🟡 The Questionable1. Premature abstraction for "future variants"// mailbox.ts:54-64
export type MailboxArtifactConfig = MailboxConfig; // Why?
export interface MailboxArtifactConfigs {
mailbox: MailboxConfig; // Only one variant
}
export type MailboxType = keyof MailboxArtifactConfigs; // "mailbox" literalYou have ONE mailbox type. But you've built a discriminated union infrastructure "to support future variants." The CLAUDE.md explicitly warns against this:
Same issue in 2. Thin wrapper methods that add no value// RadixMailboxArtifactManager
async readMailbox(address: string) {
const reader = this.createReader('mailbox');
return reader.read(address);
}This instantiates a reader just to call one method. Why not just: async readMailbox(address: string) {
return new RadixMailboxReader(this.gateway).read(address);
}The 3. Inconsistent file organizationValidator announce:
Mailbox:
Why is validator announce's query+tx combined but mailbox's separated? Why is mailbox's reader+writer combined but validator announce's separated? Pick a pattern and stick with it. 4. Unused interface// mailbox.ts:70-74
export type IMailboxArtifactManager = IArtifactManager<
MailboxType,
MailboxArtifactConfigs,
DeployedMailboxAddress
>;This interface is defined but never implemented or used. It's just sitting there "for consistency." Delete it or use it. 5. The "Raw" vs "Compound" type distinctionI get it -
For a codebase that values simplicity and explicitly warns against "enterprise-style patterns," this is a lot of abstraction machinery. Is the complexity justified? 🔴 The BadZero address handling is a breaking changePR description says this is a "drive-by change":
This could break existing code that checks: if (mailbox.defaultIsm !== undefined) { ... }Now it's always defined (as zero address). Did you verify nothing breaks? This should be in the "Breaking changes" section, not "Drive-by changes." typescript/radix-sdk/src/clients/provider.ts:195-202 Misleading PR descriptionYou say you "moved 🤔 The Philosophical QuestionLooking at the broader artifact API pattern across this PR and the codebase... is this over-engineered? The artifact state machine: This feels like bringing Java/C# enterprise patterns to TypeScript. The CLAUDE.md says:
I'm not saying scrap it - you've implemented the existing pattern consistently. But the pattern itself might not align with the stated philosophy. Worth discussing with the team. 📝 Minor Nitpicks
✅ VerdictCode quality: 8/10 - Solid TypeScript, good tests, no type safety issues. Recommendation: Merge with caution. Verify the zero address change doesn't break anything. Consider simplifying the type machinery in future iterations. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds mailbox and validator-announce artifact types, readers/writers, Radix artifact managers, a composite CoreArtifactReader, optional-field helpers, and an isEmptyAddress utility across provider-sdk, deploy-sdk, radix-sdk, and utils. It also adds end-to-end tests and package exports for the new modules. Changes
Sequence Diagram(s)mermaid CoreReader->>RawMailboxMgr: readRawMailbox(address) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.changeset/mailbox-validator-announce-artifacts.md (1)
5-5: Use past tense in changeset description, would ya?The guidelines say changesets should describe what changed in past tense. "establish" should be "established".
📝 Suggested fix
-Added mailbox and validator announce artifact interfaces in provider-sdk. The new interfaces establish the contract for mailbox and validator announce artifacts, including MailboxConfig with ISM and Hook artifact references, ValidatorAnnounceConfig with mailbox address reference, and raw artifact variants for protocol implementation use. +Added mailbox and validator announce artifact interfaces in provider-sdk. The new interfaces established the contract for mailbox and validator announce artifacts, including MailboxConfig with ISM and Hook artifact references, ValidatorAnnounceConfig with mailbox address reference, and raw artifact variants for protocol implementation use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/mailbox-validator-announce-artifacts.md at line 5, Update the changeset description to use past tense: change "establish" to "established" and adjust any related verbs (e.g., "include" -> "included") so the sentence reads that the new interfaces established the contract for mailbox and validator announce artifacts, mentioning MailboxConfig, ValidatorAnnounceConfig, ISM, and Hook artifact references in past tense.typescript/radix-sdk/src/mailbox/mailbox.ts (1)
113-125: Inconsistent comparison pattern here - let me tell ya, consistency is like swamp water, it flows better when it's all the same.Line 114 uses direct
!==comparison for the ISM address, but hooks at lines 128-134 and 149-155 useeqOptionalAddress(). For Radix addresses, the latter is more robust. Keep it consistent.♻️ Suggested fix
// Set default ISM (only if not zero address) - if (defaultIsmAddress !== ZERO_ADDRESS_HEX_32) { + if ( + !eqOptionalAddress( + defaultIsmAddress, + ZERO_ADDRESS_HEX_32, + eqAddressRadix, + ) + ) { const setIsmTx = await getSetMailboxDefaultIsmTx(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/radix-sdk/src/mailbox/mailbox.ts` around lines 113 - 125, The code currently compares defaultIsmAddress to ZERO_ADDRESS_HEX_32 using !==; change this to use the Radix address helper for consistency and robustness by replacing the check with a call to eqOptionalAddress (e.g., if (!eqOptionalAddress(defaultIsmAddress, ZERO_ADDRESS_HEX_32))) before building and broadcasting the set-ISM transaction (getSetMailboxDefaultIsmTx / this.signer.signAndBroadcast); this aligns with the other checks in the file and prevents subtle address-comparison issues..changeset/radix-validator-announce-artifacts.md (1)
5-5: Mixed tenses in this here changeset.The description mixes past tense ("Implemented", "Combined") with present tense ("provides", "fetches", "creates"). Per coding guidelines, changeset descriptions should use past tense throughout describing what changed.
📜 Suggested past-tense revision
-Implemented validator announce artifact manager, reader, and writer for Radix SDK. Combined query and transaction functions in a unified validator-announce.ts file. RadixValidatorAnnounceArtifactManager provides read and create capabilities for validator announce deployments. The reader fetches the mailbox address from chain, and the writer creates new validator announce contracts (immutable, no update operations). Includes comprehensive e2e test coverage. +Implemented validator announce artifact manager, reader, and writer for Radix SDK. Combined query and transaction functions in a unified validator-announce.ts file. RadixValidatorAnnounceArtifactManager provided read and create capabilities for validator announce deployments. The reader fetched the mailbox address from chain, and the writer created new validator announce contracts (immutable, no update operations). Included comprehensive e2e test coverage.As per coding guidelines: "Write changeset descriptions in past tense describing what changed, not imperative form"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/radix-validator-announce-artifacts.md at line 5, The changeset message uses mixed tenses; update the description to past tense throughout (e.g., change "provides", "fetches", "creates" to "provided", "fetched", "created") and make the verbs consistent with the opening past-tense verbs ("Implemented", "Combined"); specifically revise the lines mentioning RadixValidatorAnnounceArtifactManager, validator-announce.ts, the reader and the writer so the description uniformly describes what was changed in past tense.typescript/deploy-sdk/src/core/core-artifact-reader.ts (1)
18-20: Import ordering is a wee bit swampy.External package imports typically come before local imports, but
ismArtifactToDerivedConfigfrom@hyperlane-xyz/provider-sdk/ismis placed after local imports. Move it up to join its kin.🧅 Suggested reorder
-import { HookReader, createHookReader } from '../hook/hook-reader.js'; -import { IsmReader, createIsmReader } from '../ism/generic-ism.js'; -import { ismArtifactToDerivedConfig } from '@hyperlane-xyz/provider-sdk/ism'; +import { ismArtifactToDerivedConfig } from '@hyperlane-xyz/provider-sdk/ism'; + +import { HookReader, createHookReader } from '../hook/hook-reader.js'; +import { IsmReader, createIsmReader } from '../ism/generic-ism.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/deploy-sdk/src/core/core-artifact-reader.ts` around lines 18 - 20, Move the external import ismArtifactToDerivedConfig from `@hyperlane-xyz/provider-sdk/ism` so it appears with the other external package imports (before the local imports HookReader/createHookReader and IsmReader/createIsmReader); specifically, relocate the ismArtifactToDerivedConfig import line to the top import group so external imports are grouped together and local imports follow.typescript/radix-sdk/src/tests/mailbox.e2e-test.ts (1)
48-49: Reaching into private swamp territory withas any.Accessing
gatewayandbasevia(radixSigner as any)breaks encapsulation and violates the guideline againstas anycasts. If these are needed for artifact manager construction, consider either:
- Exposing public getters on
RadixSigner- Having the artifact manager accept
RadixSignerdirectly and extract what it needs internallyAs per coding guidelines: "Do not use
as anyin TypeScript; useunknownwith type guards if type is truly unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/radix-sdk/src/tests/mailbox.e2e-test.ts` around lines 48 - 49, The test is reaching into RadixSigner internals via (radixSigner as any).gateway and .base which breaks encapsulation; instead either add public getters on RadixSigner (e.g., getGateway(), getBase()) and use those in mailbox.e2e-test.ts, or change the artifact manager constructor to accept a RadixSigner instance (e.g., new ArtifactManager(radixSigner)) and let ArtifactManager extract gateway/base internally; if the type really is uncertain, use unknown with a type guard that narrows to an interface exposing gateway/base rather than using as any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/orange-socks-return.md:
- Line 6: Rewrite the changeset body to past tense: change verbs like "Takes",
"returns", "Provides", "adds", and "exports" to past-tense forms so the
description reads as what changed. Specifically, update the sentence describing
CoreArtifactReader, the mailbox handling and returned MailboxConfig, the
deriveCoreConfig() helper, the mailboxArtifactToDerivedCoreConfig conversion in
mailbox.ts, and the exported ismArtifactToDerivedConfig from the ISM reader to
past-tense phrasing.
In `@typescript/radix-sdk/src/tests/validator-announce.e2e-test.ts`:
- Around line 37-50: RadixSigner tests currently reach into protected fields
using "as any" to grab gateway and base; add two public getter methods on
RadixSigner — getGateway() returning the existing gateway field and getBase()
returning the existing base field (similar to the existing getBaseSigner()),
then update tests to call radixSigner.getGateway() and radixSigner.getBase()
instead of casting to any.
---
Nitpick comments:
In @.changeset/mailbox-validator-announce-artifacts.md:
- Line 5: Update the changeset description to use past tense: change "establish"
to "established" and adjust any related verbs (e.g., "include" -> "included") so
the sentence reads that the new interfaces established the contract for mailbox
and validator announce artifacts, mentioning MailboxConfig,
ValidatorAnnounceConfig, ISM, and Hook artifact references in past tense.
In @.changeset/radix-validator-announce-artifacts.md:
- Line 5: The changeset message uses mixed tenses; update the description to
past tense throughout (e.g., change "provides", "fetches", "creates" to
"provided", "fetched", "created") and make the verbs consistent with the opening
past-tense verbs ("Implemented", "Combined"); specifically revise the lines
mentioning RadixValidatorAnnounceArtifactManager, validator-announce.ts, the
reader and the writer so the description uniformly describes what was changed in
past tense.
In `@typescript/deploy-sdk/src/core/core-artifact-reader.ts`:
- Around line 18-20: Move the external import ismArtifactToDerivedConfig from
`@hyperlane-xyz/provider-sdk/ism` so it appears with the other external package
imports (before the local imports HookReader/createHookReader and
IsmReader/createIsmReader); specifically, relocate the
ismArtifactToDerivedConfig import line to the top import group so external
imports are grouped together and local imports follow.
In `@typescript/radix-sdk/src/mailbox/mailbox.ts`:
- Around line 113-125: The code currently compares defaultIsmAddress to
ZERO_ADDRESS_HEX_32 using !==; change this to use the Radix address helper for
consistency and robustness by replacing the check with a call to
eqOptionalAddress (e.g., if (!eqOptionalAddress(defaultIsmAddress,
ZERO_ADDRESS_HEX_32))) before building and broadcasting the set-ISM transaction
(getSetMailboxDefaultIsmTx / this.signer.signAndBroadcast); this aligns with the
other checks in the file and prevents subtle address-comparison issues.
In `@typescript/radix-sdk/src/tests/mailbox.e2e-test.ts`:
- Around line 48-49: The test is reaching into RadixSigner internals via
(radixSigner as any).gateway and .base which breaks encapsulation; instead
either add public getters on RadixSigner (e.g., getGateway(), getBase()) and use
those in mailbox.e2e-test.ts, or change the artifact manager constructor to
accept a RadixSigner instance (e.g., new ArtifactManager(radixSigner)) and let
ArtifactManager extract gateway/base internally; if the type really is
uncertain, use unknown with a type guard that narrows to an interface exposing
gateway/base rather than using as any.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4af685ec-c655-4a5b-b358-0884c72d565c
📒 Files selected for processing (22)
.changeset/mailbox-validator-announce-artifacts.md.changeset/orange-socks-return.md.changeset/radix-mailbox-artifacts.md.changeset/radix-validator-announce-artifacts.mdtypescript/deploy-sdk/src/core/core-artifact-reader.tstypescript/deploy-sdk/src/index.tstypescript/deploy-sdk/src/ism/generic-ism.tstypescript/provider-sdk/package.jsontypescript/provider-sdk/src/mailbox.tstypescript/provider-sdk/src/validator-announce.tstypescript/radix-sdk/src/clients/provider.tstypescript/radix-sdk/src/index.tstypescript/radix-sdk/src/mailbox/mailbox-artifact-manager.tstypescript/radix-sdk/src/mailbox/mailbox-query.tstypescript/radix-sdk/src/mailbox/mailbox.tstypescript/radix-sdk/src/tests/mailbox.e2e-test.tstypescript/radix-sdk/src/tests/validator-announce.e2e-test.tstypescript/radix-sdk/src/utils/base-query.tstypescript/radix-sdk/src/validator-announce/validator-announce-artifact-manager.tstypescript/radix-sdk/src/validator-announce/validator-announce-reader.tstypescript/radix-sdk/src/validator-announce/validator-announce-writer.tstypescript/radix-sdk/src/validator-announce/validator-announce.ts
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Brutally Honest Review
This review was produced by running multiple parallel review agents (provider-sdk interfaces, radix-sdk implementation, deploy-sdk architecture, tests/overall) and cross-referencing with an independent Codex review. The architecture direction is sound, but the PR overclaims backward compatibility and under-tests the only part that actually matters.
Blockers
| # | Issue | Severity |
|---|---|---|
| 1 | CoreArtifactReader creates hook reader without context.mailbox — AltVMCoreReader.ts:39 and WarpReader:57 both pass { mailbox } to createHookReader(). This new reader does not. SVM hook detection (svm-sdk/src/clients/protocol.ts:56-61) requires it. This is a functional regression on Sealevel. |
CRITICAL |
| 2 | CoreArtifactReader.read() cannot read mailboxes with zero-address ISM/hooks — the very state this PR creates (tested in mailbox.e2e-test.ts:89). read() blindly expands zero addresses via ismReader.read(ZERO_ADDRESS) / hookReader.read(ZERO_ADDRESS), and mailboxArtifactToDerivedCoreConfig then rejects anything that isn't fully DEPLOYED. Net: the new reader is incompatible with the new writer. |
CRITICAL |
| 3 | Zero test coverage for CoreArtifactReader — the main architectural addition (composite reader + deriveCoreConfig() backward-compat) has no unit or e2e tests. This is exactly why issues #1 and #2 slipped through. |
CRITICAL |
Should Fix
| # | Issue | Severity |
|---|---|---|
| 4 | create() returns config with the requested owner, not the actual on-chain owner (the deployer). Misleads callers when config.owner !== signer. |
MAJOR |
| 5 | getCreateValidatorAnnounceTx duplicated verbatim in validator-announce.ts alongside existing validator-announce-tx.ts. Writer imports the new copy; existing provider.ts/signer.ts import the old. Drift bait. |
MAJOR |
| 6 | Inconsistent zero-address comparison in RadixMailboxWriter.create(): ISM uses raw !==, hooks use eqOptionalAddress(). Different semantics (normalization, undefined handling). |
MAJOR |
| 7 | Mailbox update tests (mailbox.e2e-test.ts:208, mailbox.e2e-test.ts:258) only inspect generated tx metadata, never execute the transactions or verify final on-chain state. |
MAJOR |
Other Issues
| # | Issue | Severity |
|---|---|---|
| 8 | mailboxArtifactToDerivedCoreConfig return type is inlined instead of referencing DerivedCoreConfig — will silently diverge if DerivedCoreConfig gains fields |
MINOR |
| 9 | ValidatorAnnounceConfig and RawValidatorAnnounceConfig are byte-for-byte identical — the full Raw parallel hierarchy is pure ceremony for a config with one string field |
MINOR |
| 10 | Precondition checks in mailboxArtifactToDerivedCoreConfig use throw new Error() instead of assert() per repo convention |
MINOR |
| 11 | 17 commits including 4 merge commits, a typo (rmeove), and chore: make linter happy — needs squash/rebase |
NIT |
| 12 | Commit 7198dea has "DO NOT MERGE YET" in the title from a stacked PR — confusing in the history |
NIT |
What's Good
- Architecture direction is correct: composite reader delegates to specialized readers
ismArtifactToDerivedConfigdedup fromgeneric-ism.tsis a clean win- Validator announce correctly modeled as immutable (
update()returns[]) - Radix e2e test breadth is solid: create, read, update, no-op, zero-address skip
- Reader/writer inheritance pattern (writer extends reader) is consistent with existing ISM/hook code
Observations Outside This PR
AltVMCoreReader.ts:39: passes{ mailbox: mailboxAddress }tocreateHookReader()— this is the patternCoreArtifactReadermust followWarpReader:57: same pattern — creates hook reader insideread()with mailbox context, not in constructorvalidator-announce-tx.ts:6: existinggetCreateValidatorAnnounceTxthat is now duplicated
Review produced by multi-agent analysis (4 parallel review teams) + Codex cross-reference.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
typescript/provider-sdk/src/mailbox.ts (1)
147-162:⚠️ Potential issue | 🟡 MinorSwap these invariant throws to
assert().These are invariant checks, not recoverable control flow.
assert()keeps this helper aligned with the rest of provider-sdk.As per coding guidelines, "Use `assert()` from '@hyperlane-xyz/utils' for validating preconditions, invariants, and unexpected states rather than throwing custom errors".Suggested tweak
+import { assert } from '@hyperlane-xyz/utils'; + // All nested artifacts should be in DEPLOYED state after CoreArtifactReader.read() - if (!isArtifactDeployed(defaultIsm)) { - throw new Error( - `Expected defaultIsm to be ${ArtifactState.DEPLOYED}, got ${defaultIsm.artifactState}`, - ); - } - if (!isArtifactDeployed(defaultHook)) { - throw new Error( - `Expected defaultHook to be ${ArtifactState.DEPLOYED}, got ${defaultHook.artifactState}`, - ); - } - if (!isArtifactDeployed(requiredHook)) { - throw new Error( - `Expected requiredHook to be ${ArtifactState.DEPLOYED}, got ${requiredHook.artifactState}`, - ); - } + assert( + isArtifactDeployed(defaultIsm), + `Expected defaultIsm to be ${ArtifactState.DEPLOYED}, got ${defaultIsm.artifactState}`, + ); + assert( + isArtifactDeployed(defaultHook), + `Expected defaultHook to be ${ArtifactState.DEPLOYED}, got ${defaultHook.artifactState}`, + ); + assert( + isArtifactDeployed(requiredHook), + `Expected requiredHook to be ${ArtifactState.DEPLOYED}, got ${requiredHook.artifactState}`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/provider-sdk/src/mailbox.ts` around lines 147 - 162, Replace the three throw Error invariant checks with assert calls from '@hyperlane-xyz/utils': use assert(isArtifactDeployed(defaultIsm), `Expected defaultIsm to be ${ArtifactState.DEPLOYED}, got ${defaultIsm.artifactState}`) and similarly for defaultHook and requiredHook; update imports to include assert if not already present and remove the corresponding throw blocks so invariants use assert() instead of throwing custom Error.typescript/deploy-sdk/src/core/core-artifact-reader.ts (1)
78-89:⚠️ Potential issue | 🟠 Major
deriveCoreConfig()still breaks on empty nested refs.Lines 80-88 now keep zero-address
defaultIsm/hooks as UNDERIVED artifacts, but Lines 116-119 still feed them intomailboxArtifactToDerivedCoreConfig(), which only accepts DEPLOYED nested artifacts. So a mailbox this PR can create/read still can’t round-trip through the backward-compatible API when one of those refs is empty. Needs a regression test for that zero-address path too.Also applies to: 113-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/deploy-sdk/src/core/core-artifact-reader.ts` around lines 78 - 89, deriveCoreConfig() currently passes UNDERIVED nested artifacts (rawMailbox.config.defaultIsm / defaultHook / requiredHook) into mailboxArtifactToDerivedCoreConfig(), which expects DEPLOYED nested artifacts; fix by, when isEmptyAddress(defaultIsmAddr|defaultHookAddr|requiredHookAddr) is true, wrapping the rawMailbox.config.* values into a DEPLOYED-style artifact (a minimal deployed artifact with the zero address and required type/metadata) instead of passing the raw underived object so mailboxArtifactToDerivedCoreConfig() receives deployed artifacts; update the logic around defaultIsmArtifact/defaultHookArtifact/requiredHookArtifact (the Promise.all branch using isEmptyAddress, ismReader.read, hookReader.read) and add a regression test for the zero-address path to ensure deriveCoreConfig() round-trips through the backward-compatible API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/radix-sdk/src/mailbox/mailbox.ts`:
- Around line 50-66: The fallback logic for mailbox addresses uses the ||
operator which treats empty strings as falsy; update the deployed.address
assignments for defaultIsm, defaultHook, and requiredHook to use the nullish
coalescing operator (??) with mailboxConfig (i.e., replace
mailboxConfig.defaultIsm || ZERO_ADDRESS_HEX_32, mailboxConfig.defaultHook ||
ZERO_ADDRESS_HEX_32, and mailboxConfig.requiredHook || ZERO_ADDRESS_HEX_32 with
mailboxConfig.defaultIsm ?? ZERO_ADDRESS_HEX_32, mailboxConfig.defaultHook ??
ZERO_ADDRESS_HEX_32, and mailboxConfig.requiredHook ?? ZERO_ADDRESS_HEX_32) so
only undefined/null fallbacks to ZERO_ADDRESS_HEX_32 (getMailboxConfig returns
string | undefined).
---
Duplicate comments:
In `@typescript/deploy-sdk/src/core/core-artifact-reader.ts`:
- Around line 78-89: deriveCoreConfig() currently passes UNDERIVED nested
artifacts (rawMailbox.config.defaultIsm / defaultHook / requiredHook) into
mailboxArtifactToDerivedCoreConfig(), which expects DEPLOYED nested artifacts;
fix by, when isEmptyAddress(defaultIsmAddr|defaultHookAddr|requiredHookAddr) is
true, wrapping the rawMailbox.config.* values into a DEPLOYED-style artifact (a
minimal deployed artifact with the zero address and required type/metadata)
instead of passing the raw underived object so
mailboxArtifactToDerivedCoreConfig() receives deployed artifacts; update the
logic around defaultIsmArtifact/defaultHookArtifact/requiredHookArtifact (the
Promise.all branch using isEmptyAddress, ismReader.read, hookReader.read) and
add a regression test for the zero-address path to ensure deriveCoreConfig()
round-trips through the backward-compatible API.
In `@typescript/provider-sdk/src/mailbox.ts`:
- Around line 147-162: Replace the three throw Error invariant checks with
assert calls from '@hyperlane-xyz/utils': use
assert(isArtifactDeployed(defaultIsm), `Expected defaultIsm to be
${ArtifactState.DEPLOYED}, got ${defaultIsm.artifactState}`) and similarly for
defaultHook and requiredHook; update imports to include assert if not already
present and remove the corresponding throw blocks so invariants use assert()
instead of throwing custom Error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4c95182-1cf7-44f2-a92f-2bccb5ee0a9d
📒 Files selected for processing (11)
.changeset/orange-socks-return.md.changeset/utils-is-empty-address.mdtypescript/deploy-sdk/src/core/core-artifact-reader.tstypescript/provider-sdk/src/mailbox.tstypescript/provider-sdk/src/validator-announce.tstypescript/radix-sdk/src/mailbox/mailbox.tstypescript/radix-sdk/src/validator-announce/validator-announce-artifact-manager.tstypescript/radix-sdk/src/validator-announce/validator-announce-query.tstypescript/radix-sdk/src/validator-announce/validator-announce.tstypescript/utils/src/addresses.tstypescript/utils/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- typescript/radix-sdk/src/validator-announce/validator-announce-artifact-manager.ts
- .changeset/orange-socks-return.md
- typescript/provider-sdk/src/validator-announce.ts
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Re-Review: Post-Fix Assessment
Follow-up review after commits db8102c and 8cb9702. 6 of 12 original issues were addressed. Good progress.
Fixed
| # | Issue | Was |
|---|---|---|
| 1 | Hook reader now created per-read with context.mailbox |
CRITICAL |
| 2 | read() skips expansion for empty addresses via isEmptyAddress() |
CRITICAL |
| 4 | create() returns this.signer.getAddress() as actual owner |
MAJOR |
| 5 | Validator announce writer imports from existing validator-announce-tx.js |
MAJOR |
| 6 | ISM check in create() now uses isEmptyAddress() consistently |
MAJOR |
| 8 | mailboxArtifactToDerivedCoreConfig return type explicit |
MINOR |
Plus: new isEmptyAddress utility in @hyperlane-xyz/utils with changeset.
Still Needs Work
See inline comments. Summary:
deriveCoreConfig()still breaks on zero-address mailboxes —read()was fixed to return UNDERIVED artifacts, butderiveCoreConfig()passes them tomailboxArtifactToDerivedCoreConfig()which throws on non-DEPLOYED. Half-fix.- Zero test coverage for
CoreArtifactReader— main architectural addition, still unproven. throwvsassert()marked "fixed" but unchanged — cosmetic but misleading.
Acknowledged Won't-Fix
as anyin tests — acceptedRawValidatorAnnounceConfigduplication — intentional||vs??in reader fallbacks — intentional
Other Observations
- Update tests ("should update mailbox when ISM changes", "should update mailbox when owner changes") only check returned tx array metadata. They never execute the transactions or verify on-chain state changed. Low priority but worth noting.
- Import ordering in
core-artifact-reader.ts:20— external import after local imports.
Verdict
Significant improvement. The read() path is now correct. But deriveCoreConfig() — the backward-compat API this PR provides — still fails on zero-address mailboxes. That plus zero test coverage means the main addition is unproven. Not ready to approve.
|
Follow-up note on the re-review: Apologies — the The |
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Approved
All issues from the original and re-review are now either fixed or explicitly deferred to #8147:
Fixed in this PR
- ✅ Hook reader created per-read with
context.mailbox - ✅
read()skips expansion for empty addresses viaisEmptyAddress() - ✅
create()returns actual signer address as owner - ✅ Validator announce deduplication resolved
- ✅ Consistent
isEmptyAddress()increate() - ✅
DerivedCoreConfigreturn type explicit - ✅
assert()instead ofthrow new Error()for invariant checks (b8ca2ef)
Deferred to #8147 (acknowledged by author)
deriveCoreConfig()zero-address handlingCoreArtifactReadertest coverage
The read() path — the core addition — is correct. The backward-compat deriveCoreConfig() edge case has a clear follow-up plan. Ship it.
…to andrey/mailbox-validator-artifact-interfaces
3 participants
Description
Added mailbox and validator announce artifact interfaces to
provider-sdk, implemented Radix-specific artifact managers (reader/writer) inradix-sdk, and added a compositeCoreArtifactReaderindeploy-sdkthat orchestrates mailboxprovider-sdk:
mailbox.ts:MailboxConfig,DeployedMailboxArtifact,IRawMailboxArtifactManager,mailboxArtifactToDerivedCoreConfigconvertervalidator-announce.ts:ValidatorAnnounceConfig,IRawValidatorAnnounceArtifactManagerradix-sdk:
RadixMailboxArtifactManagerwith reader (fetches on-chain config) and writer (create/update mailbox)RadixValidatorAnnounceArtifactManagerwith reader and writer (create only, immutable)deploy-sdk:
CoreArtifactReader: composite reader that takes a mailbox address, reads raw config, then expands nested ISM/hook artifacts via existing readers. Provides backward-compatiblederiveCoreConfig().Drive-by changes
ismArtifactToDerivedConfigfromdeploy-sdkgeneric-ism.ts toprovider-sdkism.ts (removed duplication after rebase)RadixProvider.getMailboxnow returns zero addresses instead of undefined for optional mailbox fields (defaultIsm,defaultHook,requiredHook)provider-sdkmailbox query types (ResGetMailbox) updated to support optional fields with zero address fallbackRelated issues
None
Backward compatibility
Yes — new interfaces and implementations only, no breaking changes to existing APIs. The
ismArtifactToDerivedConfigmove is re-exported from the same entry point.Testing
E2e tests for Radix mailbox and validator announce artifact managers.
Summary by CodeRabbit
New Features
Tests
Chores