fix receiver should use reserve and carry_all_balance#631
fix receiver should use reserve and carry_all_balance#631vicentevieytes wants to merge 1 commit intomainfrom
Conversation
|
👋 vicentevieytes, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
Updates the CCIP test receiver contract to preserve its original TON balance when handling ccipReceive, aligning the confirmation-send behavior with the intended reserve + carry-all-balance pattern.
Changes:
- Add a regression test asserting the receiver’s balance is unchanged after a successful
ccipReceive. - Update the test receiver contract to reserve the original balance and send the confirmation with
SEND_MODE_CARRY_ALL_BALANCE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
contracts/tests/ccip/Receiver.spec.ts |
Adds a balance-preservation regression test for successful ccipReceive. |
contracts/contracts/ccip/test/receiver/contract.tolk |
Reserves original balance and changes confirmation send mode to carry all balance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -49,6 +38,20 @@ fun onInternalMessage(in: InMessage) { | |||
| processConsumeAllGas() | |||
| } | |||
| } | |||
|
|
|||
| reserveToncoinsOnBalance(0, RESERVE_MODE_INCREASE_BY_ORIGINAL_BALANCE); | |||
| msg.validateAndConfirm({ | |||
There was a problem hiding this comment.
processAccept / processRejectAll / processConsumeAllGas runs before validateAndConfirm (router + minValue checks). This changes semantics (e.g., unauthorized/low-value messages can now hit ConsumeAllGas and fail with out-of-gas instead of Unauthorized/LowValue) and can burn gas before failing. Validate the sender/value first (or split validation from confirmation), then run behavior, then reserve+send confirmation so failures are deterministic and cheap.
There was a problem hiding this comment.
@patricios-space hmm this is true, the minValue check should happen at the very beginning, but setting the action for the confirm message with this send mode and the reserve at the very beginning fails to preserve the original balance.
There was a problem hiding this comment.
Yes, I was about to mention this. Then we should split validateAndConfirm, runing the validate before, and confirm after.
| }) | ||
| }) | ||
|
|
||
| it('should keep original balance after succesfully receiving', async () => { |
There was a problem hiding this comment.
Typo in test name: "succesfully" should be "successfully".
| it('should keep original balance after succesfully receiving', async () => { | |
| it('should keep original balance after successfully receiving', async () => { |
| processConsumeAllGas() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I had to move this validateAndConfirm code block under the process functions. Turns out that if you do the reserve and send trick before the emit then it does not work, it has to be the last message sent. @patricios-space
| @@ -49,6 +38,20 @@ fun onInternalMessage(in: InMessage) { | |||
| processConsumeAllGas() | |||
| } | |||
| } | |||
|
|
|||
| reserveToncoinsOnBalance(0, RESERVE_MODE_INCREASE_BY_ORIGINAL_BALANCE); | |||
| msg.validateAndConfirm({ | |||
There was a problem hiding this comment.
Yes, I was about to mention this. Then we should split validateAndConfirm, runing the validate before, and confirm after.
|
|
||
| const finalBalance = (await blockchain.getContract(receiver.address)).balance | ||
| expect(finalBalance).toEqual(initialBalance) | ||
| }) |
There was a problem hiding this comment.
This is a flaky test because storage fee could be charged on message delivery. This will take it into consideration:
| const finalBalance = (await blockchain.getContract(receiver.address)).balance | |
| expect(finalBalance).toEqual(initialBalance) | |
| }) | |
| const tx = result.transactions.find( | |
| (tx) => | |
| tx.inMessage && | |
| tx.inMessage.info.src && | |
| tx.inMessage.info.src instanceof Address && | |
| tx.inMessage.info.src.equals(deployer.address) && | |
| tx.inMessage.info.dest && | |
| tx.inMessage.info.dest instanceof Address && | |
| tx.inMessage.info.dest.equals(receiver.address), | |
| ) | |
| if (!tx || tx.description.type != 'generic') { | |
| throw new Error('Expected an internal message') | |
| } | |
| const storageFees = tx.description.storagePhase?.storageFeesCollected || toNano('0') | |
| const finalBalance = (await blockchain.getContract(receiver.address)).balance | |
| expect(finalBalance).toEqual(initialBalance - storageFees) | |
| }) |
No description provided.