refactor: optional mailbox on warp config #5500
Conversation
mshojaei-txfusion
requested review from
yorhodes,
ltyu and
paulbalaji
as code owners
🦋 Changeset detectedLatest commit: 1c322ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
mshojaei-txfusion
changed the title
mshojaei-txfusion
requested review from
tkporter and
Mo-Hussain
as code owners
typescript/infra/config/warp.ts
Outdated
| @@ -125,6 +125,8 @@ async function getConfigFromMergedRegistry( | |||
| true, | |||
| ).getWarpDeployConfig(warpRouteId); | |||
| assert(warpRoute, `Warp route Config not found for ${warpRouteId}`); | |||
| //TODO: fix this and manage mailbox | |||
There was a problem hiding this comment.
@nambrot Can you please take a look at this?
should I handle mailbox on infra side or on registry itself?
So I need to do similar thing fillDefaults did on cli.
There was a problem hiding this comment.
As mailbox may not return when we call getWarpDeployConfig from registry
There was a problem hiding this comment.
I think the Registry should handle this, given that the mailbox address comes from the Registry
There was a problem hiding this comment.
on second thought, i am kind of leaning towards keeping that Registry function as is i.e. to fetch the non-derived.
do the callers actually need the mailbox here?
There was a problem hiding this comment.
This is a great question, I'm inclined to change the return type here and then see where it breaks. Generally speaking would prefer to pass around the "simpler" version and then only "hydrate" the config when necessary
There was a problem hiding this comment.
I believe this would be needed on deploy script of infra, so we really needed and should decide for the way we get it.
There was a problem hiding this comment.
yeah lets keep the Registry as is and then do some sort of filldefault() here
There was a problem hiding this comment.
got it, done!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5500 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
🚀 New features to boost your workflow:
|
…ve ts-ignore workaround
Description
This PR updates the warp route deployment configuration to handle mailbox addresses more consistently. Key changes:
Drive-by changes
Related issues
#5258
Backward compatibility
Yes - This change maintains backward compatibility while improving the internal handling of mailbox addresses.
Testing
Unit Tests - Existing tests have been updated to accommodate the new mailbox handling approach