feat(deploy-sdk): deploy sdk core artifact api implementation#8147
feat(deploy-sdk): deploy sdk core artifact api implementation#8147
Conversation
80217ce to
c44aa22
Compare
…protocol provider - Add createMailboxArtifactManager() and createValidatorAnnounceArtifactManager() to ProtocolProvider interface - Implement methods in RadixProtocolProvider with getCommonSetup() helper - Add stub implementations for AleoProtocolProvider and TronProtocolProvider (throws 'Not implemented') - Add stub implementation for CosmosNativeProtocolProvider (returns null for VA as Cosmos doesn't support it) - Refactor existing RadixProtocolProvider methods to use getCommonSetup() helper Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
… deployment - Create CoreWriter class extending CoreArtifactReader - Implement create() method following artifact API pattern (like WarpTokenWriter) - Deploy ISM if NEW, use existing if DEPLOYED/UNDERIVED - Create mailbox with ISM initially without hooks - Deploy hooks if NEW (require mailbox address for context) - Update mailbox configuration with hooks and transfer owner using signer - Deploy validator announce if supported by protocol - Return deployed mailbox and validator announce artifacts with receipts - Add detailed logging with chain names for better visibility Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
- Implement update() method following WarpTokenWriter pattern - Read current mailbox state with fully expanded ISM/hook artifacts - Add assertions to verify artifacts are DEPLOYED after reading - Deploy or update ISM using merge pattern (preserves DEPLOYED nested ISMs) - Deploy or update hooks using merge pattern (preserves DEPLOYED nested hooks) - Update mailbox configuration if ISM/hooks/owner changed - Add helper methods: deployOrUpdateIsm(), deployOrUpdateHook() - Support routing ISM updates and IGP hook updates - Return array of update transactions Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
- Add createCoreWriter() factory function following pattern of createHookWriter/createIsmWriter - Get protocol provider and create mailbox/validator announce artifact managers - Export CoreWriter and createCoreWriter from deploy-sdk index - Enable public API access to CoreWriter functionality Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
- Change create() and update() to accept ArtifactNew<MailboxConfig> instead of MailboxOnChain - MailboxConfig uses Artifact (NEW/DEPLOYED/UNDERIVED) for nested fields - MailboxOnChain uses ArtifactOnChain (only DEPLOYED/UNDERIVED) for on-chain state - Update helper methods to handle UNDERIVED artifacts before merging - Simplify deployOrUpdateIsm/Hook: check UNDERIVED, merge, then deploy or update - Import Artifact type from artifact module Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
c44aa22 to
d06fbb8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8147 +/- ##
=======================================
Coverage 76.49% 76.49%
=======================================
Files 128 128
Lines 3416 3416
Branches 290 290
=======================================
Hits 2613 2613
Misses 786 786
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@claude review |
|
Claude finished @paulbalaji's task in 5m 47s —— View job Review: deploy-sdk core artifact API
Submitted a consolidated PR review with 5 inline comments covering:
Items already flagged by Devin ( |
📝 WalkthroughWalkthroughAdds a CoreWriter orchestrator and mailbox-scoped hook reader factory, introduces UnsetArtifactAddress and toDeployedOrUndefined helpers, extends ProtocolProvider with mailbox/validator artifact manager factories across SDKs, adjusts AltVMCoreModule for address-or-config handling, and adds comprehensive tests for reader/writer flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CoreWriter
participant ProtocolProvider
participant MailboxMgr as MailboxMgr
participant ISMMgr as ISMMgr
participant HookMgr as HookMgr
participant VAMgr as ValidatorAnnounceMgr
participant Signer
participant Chain
Client->>CoreWriter: create(mailbox artifact)
CoreWriter->>ProtocolProvider: createMailboxArtifactManager(chainMetadata)
CoreWriter->>ISMMgr: deploy ISM (if NEW)
ISMMgr->>Chain: submit ISM tx
Chain-->>ISMMgr: ISM address
CoreWriter->>MailboxMgr: create mailbox (refs ISM/address)
MailboxMgr->>Signer: sign creation
Signer->>Chain: broadcast tx
Chain-->>Signer: receipt
par Deploy hooks (if NEW)
CoreWriter->>HookMgr: deploy default hook (mailbox context)
HookMgr->>Chain: submit hook tx
HookMgr-->>CoreWriter: hook address
CoreWriter->>HookMgr: deploy required hook (mailbox context)
end
CoreWriter->>MailboxMgr: update mailbox with hooks & ISM
MailboxMgr->>Signer: sign update
Signer->>Chain: broadcast update tx
Chain-->>Signer: receipt
opt Protocol supports VA
CoreWriter->>VAMgr: deploy validator announce
VAMgr->>Chain: submit VA tx
Chain-->>VAMgr: VA address
end
CoreWriter-->>Client: return artifacts + receipts
sequenceDiagram
participant Client
participant CoreWriter
participant CoreArtifactReader
participant MailboxMgr as MailboxMgr
participant ISMMgr as ISMMgr
participant HookMgr as HookMgr
participant Chain
Client->>CoreWriter: update(expectedArtifact)
CoreWriter->>CoreArtifactReader: deriveCoreConfig()
CoreArtifactReader->>Chain: read mailbox state
CoreArtifactReader->>ISMMgr: read ISM (skip if zero address)
CoreArtifactReader->>HookMgr: read hooks (skip if zero address)
CoreWriter->>ISMMgr: deployOrUpdateIsm (if changed)
CoreWriter->>HookMgr: deployOrUpdateHook (if changed)
CoreWriter->>MailboxMgr: update mailbox config
MailboxMgr->>Chain: submit update tx
Chain-->>MailboxMgr: receipt
CoreWriter-->>Client: updated artifact + receipts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
…to xeno/deploy-sdk-core-artifact-api
paulbalaji
left a comment
There was a problem hiding this comment.
Review Summary
Clean PR that adds CoreWriter for orchestrating artifact-API-based core deployments (mailbox, ISM, hooks, validator announce). The 5-step create flow correctly handles the circular dependency between mailbox and hooks. The CoreArtifactReader composite pattern is well-structured. toDeployedOrUndefined and fromOnChainArtifact helpers follow the fail-fast assert() pattern.
Accounting for existing reviews (Claude, Devin, CodeRabbit) and the author's responses. Not re-flagging resolved items.
Key findings
| # | Severity | Finding |
|---|---|---|
| 1 | Medium | update() has mixed execution model — deploys ISMs/hooks eagerly inside but returns AnnotatedTx[] — undocumented |
| 2 | Medium | DerivedCoreConfig widening diverges from SDK's DerivedCoreConfig — type-level inconsistency |
| 3 | Medium | Hook update/deploy path in update() has zero test coverage |
| 4 | Medium | Update tests don't verify address wiring or owner changes |
| 5 | Low | toDeployedOrUndefined and fromOnChainArtifact duplicate validation logic |
| 6 | Low | Reader tests bypass constructor via Object.create — hides constructor bugs |
Notes on prior review findings
- Zero-address hook crash in AltVMCoreModule (raised by Claude/Devin): Confirmed stale —
hook-writer.ts:171now has!actualAddress || isEmptyAddress(actualAddress)guard. - Receipt loss in
deployOrUpdateIsm/Hook: Acknowledged by author as intentional per artifact API spec. The mixed execution model (finding #1) is the remaining documentation concern. - SVM/Tron throw vs null for VA: Low severity — both
createMailboxArtifactManagerandcreateValidatorAnnounceArtifactManagerthrow, socreateCoreWriterwill fail at construction for these protocols regardless.
Observations Outside This PR
typescript/sdk/src/core/types.ts: SDK'sDerivedCoreConfigusesDerivedIsmConfig(no| stringunion), while provider-sdk's now allows| UnsetArtifactAddress. These are separate types but share the same name. Consumers importing from@hyperlane-xyz/sdkwon't see the widening. CLIcore read(viaAltVMCoreReader) could emitZERO_ADDRESS_HEX_32strings. The SDK'sCoreConfigSchemaaccepts strings viaZHashso parsing won't fail at runtime, but the type-level divergence is worth tracking.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
typescript/deploy-sdk/src/core/core-writer.test.ts (1)
49-60: Trim unused protocol registry scaffolding in this test file.
TestProtocolregistration +mockProtocolProvidersetup looks unused here since tests constructCoreWriterdirectly. Dropping it would keep this test onion tighter and reduce global shared state.♻️ Proposed cleanup
-// Test protocol -const TestProtocol = 'test-core-writer' as ProtocolType; - -let mockProtocolProvider: ProtocolProvider; - -if (!hasProtocol(TestProtocol)) { - registerProtocol(TestProtocol, () => mockProtocolProvider); -} +// No protocol registration needed in this unit test file; +// tests instantiate CoreWriter directly with artifact managers.Also applies to: 143-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/deploy-sdk/src/core/core-writer.test.ts` around lines 49 - 60, Remove the unused protocol registry scaffolding: delete the TestProtocol constant, the mockProtocolProvider variable, and the conditional hasProtocol/registerProtocol block (and the duplicate scaffolding around lines 143-157) since CoreWriter tests construct instances directly; also remove any unused references to mockProtocolProvider and related protocol-registration setup to avoid global test state and keep the file self-contained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@typescript/deploy-sdk/src/core/core-writer.test.ts`:
- Around line 49-60: Remove the unused protocol registry scaffolding: delete the
TestProtocol constant, the mockProtocolProvider variable, and the conditional
hasProtocol/registerProtocol block (and the duplicate scaffolding around lines
143-157) since CoreWriter tests construct instances directly; also remove any
unused references to mockProtocolProvider and related protocol-registration
setup to avoid global test state and keep the file self-contained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68d0e9ab-8869-4983-8f46-7d48ab0a2a07
📒 Files selected for processing (3)
typescript/deploy-sdk/src/core/core-writer.test.tstypescript/deploy-sdk/src/core/core-writer.tstypescript/provider-sdk/src/mailbox.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- typescript/deploy-sdk/src/core/core-writer.ts
paulbalaji
left a comment
There was a problem hiding this comment.
All findings addressed. LGTM.
Description
This PR adds the
CoreWriterto thedeploy-sdkfor coordinating the deployment of core artifacts using the underlying protocol sdk writers.CoreWriterclass to thedeploy-sdkcreateMailboxArtifactManagerandcreateValidatorAnnounceArtifactManagermethods to theProtocolProviderinterface to get a mailbox and validator announce and updates the existing protocol providers to add placeholder methods where a core artifact implementation is not available yetDrive-by changes
Related issues
Backward compatibility
Testing
Summary by CodeRabbit
New Features
Behavior
Tests