feat: add CreditMessagingRecovery for emergency credit recovery#532
feat: add CreditMessagingRecovery for emergency credit recovery#532
Conversation
|
There was a problem hiding this comment.
Pull request overview
Adds an emergency-only credit recovery mechanism to Stargate V2 EVM messaging by introducing an owner-controlled MintBurnCreditMessaging contract that can mint credits cross-chain without local deduction and burn local credits without sending an LZ message, plus deployment + unit test updates.
Changes:
- Introduces
MintBurnCreditMessagingwithmintCredits,burnCredits, andquoteMintCredits. - Adds
IMintBurnCreditMessaginginterface for the new admin API surface. - Updates deployment to use
MintBurnCreditMessaging(non-ALT) while keeping the deployment nameCreditMessaging, and adds/updates unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stg-evm-v2/test/unitest/messaging/Messaging.MintBurnCredit.t.sol | New unit tests for mint/burn credit emergency flows and access control. |
| packages/stg-evm-v2/test/unitest/messaging/Messaging.Credit.t.sol | Adds assertions that base CreditMessaging does not expose mint/burn functions (but encoding needs fixing). |
| packages/stg-evm-v2/src/messaging/MintBurnCreditMessaging.sol | New contract extending CreditMessaging with owner-only mint/burn credit operations and fee quoting. |
| packages/stg-evm-v2/src/interfaces/IMintBurnCreditMessaging.sol | New interface for emergency credit management API (parameter locations/events need alignment with intent). |
| packages/stg-evm-v2/deploy/003-deploy-message.ts | Deploys MintBurnCreditMessaging under the CreditMessaging deployment name for non-ALT networks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…redit.t.sol Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// @dev Credits are sent to the destination chain without being deducted on the current chain. | ||
| function mintCredits( | ||
| uint32 _dstEid, | ||
| CreditBatch[] calldata _batches, | ||
| string calldata _reason | ||
| ) external payable onlyOwner { | ||
| if (bytes(_reason).length == 0) revert MintBurnCreditMessaging_EmptyReason(); | ||
| (bytes memory message, bytes memory options) = _buildMessagePayload(_dstEid, _batches); | ||
| _lzSend(_dstEid, message, options, MessagingFee(msg.value, 0), msg.sender); | ||
| emit CreditsMinted(_dstEid, _batches, _reason); | ||
| } |
There was a problem hiding this comment.
mintCredits / quoteMintCredits can be called with _batches that contain zero total credits (empty array or batches with empty credits). In that case totalCreditNum becomes 0 and _buildOptions(_dstEid, 0) produces an options payload with a 0 gas value, then mintCredits still calls _lzSend with a no-op message. Consider adding a guard to revert (or no-op without sending) when totalCreditNum == 0, mirroring CreditMessaging.sendCredits which skips _lzSend when there are no credits to send.
There was a problem hiding this comment.
intentionally omitted. mintCredits and burnCredits are emergency owner-only operations that require careful review by signers and engineers before execution, including a mandatory non-empty reason. The inputs are expected to be validated off-chain before the multisig signs. This check for dummies seems not needed imo
ravinagill15
left a comment
There was a problem hiding this comment.
Nice 🚀 let's add a changeset as well
| if (bytes(_reason).length == 0) revert MintBurnCreditMessaging_EmptyReason(); | ||
| (bytes memory message, bytes memory options) = _buildMessagePayload(_dstEid, _batches); | ||
| _lzSend(_dstEid, message, options, MessagingFee(msg.value, 0), msg.sender); | ||
| emit CreditsMinted(_dstEid, _batches, _reason); |
There was a problem hiding this comment.
Unrelated to the code, but have we thought about how the planner/indexer will handle the new event topology?
There was a problem hiding this comment.
you mean when new credits were created?
I'm not sure we have an indexer here. I think the planner just queries all the information on-chain and gets the real credits in the mesh. Will double check
There was a problem hiding this comment.
I don't know how the new events will affect any backend system that we already have, since CreditsSent and CreditsReceived are emitted.
There was a problem hiding this comment.
Just confirmed the indexer does not index credits moving
| import { IMintBurnCreditMessaging } from "../interfaces/IMintBurnCreditMessaging.sol"; | ||
| import { ICreditMessagingHandler, Credit } from "../interfaces/ICreditMessagingHandler.sol"; | ||
|
|
||
| contract MintBurnCreditMessaging is CreditMessaging, IMintBurnCreditMessaging { |
There was a problem hiding this comment.
I'd consider renaming to CreditMessagingRecoverable, CreditMessagingAdjustable, or CreditMessagingMintableBurnable to keep the consistency with Stargate contract names:
- Suffixes over prefixes. This feels like something we should respect.
- Use-cases over actions (
StargatePoolMigratableinstead ofStargatePoolBurnable). This is more subjective and I don't have a strong opinion.
We could also consider renaming mintCredits and burnCredits to mintCreditsForRecovery and burnCreditsForRecovery. No strong opinion here either, just sharing my thoughts.
There was a problem hiding this comment.
I agree, however, I renamed to CreditMessagingMintableBurnable, I agree with "Use-cases over actions" but I don't like CreditMessagingRecoverable or CreditMessagingAdjustable and I cannot find a better name that describes the use case 🤔
There was a problem hiding this comment.
Renamed to CreditMessagingRecovery it's a credit messaging contract that performs recovery.
658e380
Kept the function names as mintCredits and burnCredits for now, I don't like adding the for recovery, it seems too verbose.
I have been thinking about restoreCredits / trimCredits, first one implies bringing something back to what it should be, and second implies removing excess, both suggest the recovery intent. But not sure if it would make it harder to understand if the credits are increasing or decreasing. wdyt @tinom9 ?
There was a problem hiding this comment.
restoreCredits and trimCredits seem less verbose, what do they really mean? I prefer mint-burn.
There was a problem hiding this comment.
updated PR title and description. Keeping mint burn for now
| function _mockStargateReceiveCredits(Credit[] memory _credits) internal { | ||
| _mockStargateReceiveCredits(STARGATE_IMPL, _credits); | ||
| } | ||
|
|
||
| function _mockStargateReceiveCredits(address _stargate, Credit[] memory _credits) internal { | ||
| vm.mockCall(_stargate, abi.encodeCall(ICreditMessagingHandler.receiveCredits, (0, _credits)), ""); | ||
| } |
There was a problem hiding this comment.
I don't love that we're mocking this.
It's a fundamentally important integration of this contract.
That said, this is consistent with the codebase, so I think it's okay to keep this file as is.
Can we have some integration tests though?
There was a problem hiding this comment.
agree we should add integratio test for this
| error CreditMessagingRecovery_EmptyReason(); | ||
|
|
||
| /// @notice Emitted when credits are minted locally without a corresponding debit on a source chain. | ||
| event CreditsMinted(uint16 assetId, Credit[] credits, string reason); |
There was a problem hiding this comment.
Let's index assetId?
| event CreditsMinted(uint16 assetId, Credit[] credits, string reason); | |
| event CreditsMinted(uint16 indexed assetId, Credit[] credits, string reason); |
Same for CreditsBurned.
There was a problem hiding this comment.
asset id is removed now, it would be on each CreditBatch item
| StargateBaseTestC internal pool; | ||
|
|
||
| function setUp() public { | ||
| pool = new StargateBaseTestC(); |
There was a problem hiding this comment.
What about testing behaviour with both StargatePool and StargateOFT, instead of StargateBase mocks?
tinom9
left a comment
There was a problem hiding this comment.
Contracts look good to me!
| import { ICreditMessagingRecovery } from "../interfaces/ICreditMessagingRecovery.sol"; | ||
| import { ICreditMessagingHandler, Credit } from "../interfaces/ICreditMessagingHandler.sol"; | ||
|
|
||
| contract CreditMessagingRecovery is CreditMessaging, ICreditMessagingRecovery { |
There was a problem hiding this comment.
Only comment I would make is I dont like the name CreditMessagingRecovery, really should be something like CreditMessagingV2 imo.
Further to that, I think to simplify the system and risk of deploying on new chains, that perhaps we deploy this on arbitrum only, and not new chains moving forward. We dont need to know that before sending to audit, and also regarding the contract name, not a huge deal to change mid audit as well. The rest of impl makes sense.
Another thing to note is that currently if the planner has the contract paused, then we cant mint or burn credits. So this creates a bit of an issue if it MUST remain paused during the mint.burn recovery or migration / chain deprecation. However I think this is fine becaause worst case the owner can 1: transfer 'planner' to the owner, 2: unpause 3: mint/burncredits, 4: pause, 5: transfer planner back to the planner. This creates an atomic situation albeit, quite complicated.
There was a problem hiding this comment.
My intuition is that we only want to deploy once.
if so, I don't love CreditMessagingV2 naming, since it implies the next version (V2) of the same thing (credit messaging). I'd stick to use-case.
StargatePool->StargatePoolMigratable.CreditMessaging->CreditMessagingRecovery/CreditMessagingRecoverable.
There was a problem hiding this comment.
I also think this should be deployed on a single chain. @tironiigor’s plan was to deploy the new version from now on, but I agree we could deploy it on one of the main chains and continue using the current one elsewhere.
Regarding the name, I don’t have a strong opinion. I’m not a big fan of CreditMessagingRecovery either, but I don’t have a better suggestion. That said, I agree with @tino that if we go with a single-chain deployment, calling it CreditMessagingV2 could be misleading since we’d deploy V1 to new chains.
On the planner pausing point,good observation. The owner can ultimately take over the role and mint/burn if needed. It’s not ideal, but it’s possible, and everything can be done atomically. So I’d keep it in mind, but I don’t see it as an issue
There was a problem hiding this comment.
Yeah, fair if only 1 deployment shouldnt be v2, but we can brainstorm a better name. Igor agrees on doing it only one chain, as long as the migration (vs. just a new version on expansion) isnt too chaotic. Looking into that now
Problem
Credits in Stargate V2 act as a capacity gate — they must be held on a chain before tokens can be redeemed or sent cross-chain. When a chain is deprecated without properly returning its credits to origin chains, those credits become permanently lost. This leaves pools with real token balances but depleted path credits, blocking all user exit paths.
Example: Sei USDT pool holds $27.7k but only $4.2k in usable credits — $23.5k is effectively locked with no way out.
Credits are not a security primitive (they do not custody funds — every actual fund movement requires independent authorization via LP tokens or a valid cross-chain token message). They are a liveness primitive.
Solution
CreditMessagingRecoveryextendsCreditMessagingwith two owner-only admin functions. Both operate locally only — no LayerZero message is ever sent, no cross-chain coordination is required, and no LZ fees are incurred.mintCredits(CreditBatch[], reason)— increases credits on the current chain by callingICreditMessagingHandler.receiveCredits(0, credits)directly, bypassing the normal credit validation. Used to restore lost credits on affected chains.burnCredits(TargetCreditBatch[], reason)— decreases credits on the current chain by callingICreditMessagingHandler.sendCredits(0, targets)locally, stopping before_lzSend. UsesminAmount = amount(all-or-nothing), reverting if the path has insufficient credits. Used to correct over-minted credits.Both functions require a non-empty plain-text
reasonstring that is emitted on-chain as a permanent audit trail, as per the team discussion.Safety
mintCreditsandburnCreditsareonlyOwner(multisig), not planner-accessible.sendCreditsremains planner-accessible and unchanged.recoverTokenare unaffected — they operate onpoolBalanceSDandtreasuryFee, which credits never touch.Changes
New contract —
src/messaging/CreditMessagingRecovery.solExtends
CreditMessagingwithmintCreditsandburnCredits.New interface —
src/interfaces/ICreditMessagingRecovery.solExposes the admin functions, the
CreditMessagingRecovery_EmptyReasonerror, and theCreditsMinted/CreditsBurnedevents.Deploy script —
deploy/003-deploy-message.tsNon-alt chains now deploy
CreditMessagingRecoveryunder the existingCreditMessagingdeployment name. Alt chains continue deployingCreditMessagingAltunchanged.Unit tests —
test/unitest/messaging/Messaging.CreditRecovery.t.solExtends
CreditMessagingTestto prove backward compatibility — all parent tests run againstCreditMessagingRecovery. Recovery-specific coverage includes: access control, empty reason revert, unavailable asset revert, local-only operation (noPacketSentevent), multiple batches with multiple credits per batch, partial burns (handler returns less than requested), and empty credits arrays.Existing credit messaging tests —
test/unitest/messaging/Messaging.Credit.t.solAdded tests asserting that
mintCreditsandburnCreditsproduce an empty revert (function does not exist) on the baseCreditMessagingcontract.TBD
CreditMessagingAltis unchanged — the mesh can be balanced using a singleCreditMessagingRecoverydeployment.Mint/burn intentionally breaks this invariant — but so did the original bug: disconnecting a chain without returning credits already left the mesh in a state where total credits no longer matched pool balances. We decided against adjusting the invariant tests since these are emergency-only functions and the fuzzer is better spent on the normal credit flow. Gathering team thoughts on this.