Fix: ToB-22 (using msg.value in a loop)

[ChiTimesChi]

Description

• Fixed ToB-22 by using a custom provided tips value instead of msg.value in MessageRecipient. This should allow sending more than one message in tx for any contract that inherits from it.

Summary by CodeRabbit

• New Feature: Added a tipsValue parameter to the _sendBaseMessage function in the BaseClient, MessageRecipient, PingPongClient, TestClient, BaseClientHarness, and BaseClientTest contracts. This allows users to specify the tips to be paid for sending a message.
• Refactor: Updated the sendMessage function in PingPongClient and TestClient contracts to use named arguments and pass a struct MessageRequest to the _sendBaseMessage function, improving code readability.
• Test: Added a new test function test_sendBaseMessage_tipsValueNotMsgValue in BaseClientTest to validate the scenario where tipsValue is not equal to msg.value.

[coderabbitai]

Walkthrough

The changes primarily revolve around the addition of a tipsValue parameter to the _sendBaseMessage function across multiple contracts. This parameter represents the tips to be paid for sending the message. The function calls have been updated accordingly, and new tests have been added to validate the changes.

Changes

File Path	Summary
.../contracts/client/BaseClient.sol	Added tipsValue parameter to _sendBaseMessage function.
.../contracts/client/MessageRecipient.sol	Included tipsValue parameter in _sendBaseMessage function.
.../contracts/client/PingPongClient.sol	Modified sendMessage function to use named arguments and pass a struct to _sendBaseMessage function.
.../contracts/client/TestClient.sol	Renamed optimisticSeconds to optimisticPeriod and added tipsValue parameter in sendMessage function.
.../test/harnesses/client/BaseClientHarness.t.sol	Updated sendBaseMessage function to accept tipsValue parameter.
.../test/suite/client/BaseClient.t.sol	Added tipsValue parameter to sendBaseMessage function and added a new test function test_sendBaseMessage_tipsValueNotMsgValue.

🐇
"In the land of code, where logic is king,
A rabbit hopped forth, a new feature to bring.
With a tip of his hat, and a tweak of the dial,
He added tipsValue, with a satisfied smile.
Tests were written, the code was reviewed,
And the rabbit hopped off, his mission concluded." 🎩💻🥕

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
• Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
  resume reviews, enter @coderabbitai resume as any issue or review comment.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 2ee5234 and 340f968.

Files selected for processing (6)

• packages/contracts-core/contracts/client/BaseClient.sol (1 hunks)
• packages/contracts-core/contracts/client/MessageRecipient.sol (1 hunks)
• packages/contracts-core/contracts/client/PingPongClient.sol (1 hunks)
• packages/contracts-core/contracts/client/TestClient.sol (1 hunks)
• packages/contracts-core/test/harnesses/client/BaseClientHarness.t.sol (1 hunks)
• packages/contracts-core/test/suite/client/BaseClient.t.sol (2 hunks)

Files skipped from review due to trivial changes (2)

• packages/contracts-core/contracts/client/TestClient.sol
• packages/contracts-core/test/harnesses/client/BaseClientHarness.t.sol

Additional comments (Suppressed): 8

packages/contracts-core/contracts/client/PingPongClient.sol (1)

• 125-135: The _sendBaseMessage function call has been updated to use named arguments and includes a new tipsValue parameter. Ensure that the tipsValue is correctly set and used throughout the codebase. Also, the hardcoded MessageRequest values should be reviewed and replaced with dynamic values if necessary.

packages/contracts-core/test/suite/client/BaseClient.t.sol (3)

• 59-66: The sendBaseMessage function now includes tipsValue as an argument. Ensure that the function call is updated everywhere in the codebase. Also, verify that the tipsValue is correctly used in the function.

• 78-82: The sendBaseMessage function now includes tipsValue as an argument. Ensure that the function call is updated everywhere in the codebase. Also, verify that the tipsValue is correctly used in the function.

• 84-97: A new test function test_sendBaseMessage_tipsValueNotMsgValue is added to test the scenario where tipsValue is not equal to msg.value. Ensure that the test covers all edge cases and the assertions are correct.

packages/contracts-core/contracts/client/MessageRecipient.sol (1)

• 76-95: The function signature of _sendBaseMessage has been updated to include a new parameter tipsValue. This parameter is used as the value argument when calling the sendBaseMessage function of the InterfaceOrigin contract. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the tipsValue is always less than or equal to msg.value to prevent any potential underflow errors.

-        return InterfaceOrigin(origin).sendBaseMessage{value: msg.value}(
+        return InterfaceOrigin(origin).sendBaseMessage{value: tipsValue}(
             destination_, recipient, optimisticPeriod, _encodeRequest(request), content
         );

packages/contracts-core/contracts/client/BaseClient.sol (3)

• 78-101: The function signature of _sendBaseMessage has been updated to include a new parameter tipsValue. This parameter represents the tips to be paid for sending a message. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the tipsValue is being correctly calculated and passed in all instances where _sendBaseMessage is called.

• 93-100: The _sendBaseMessage function now uses named arguments and passes a struct to improve code readability. This is a good practice as it makes the code easier to understand and maintain.

• 91-91: The return statement is calling _sendBaseMessage recursively. Ensure that this is the intended behavior and that there is a base case to prevent infinite recursion. If there is another function with the same name but different parameters, consider renaming one of the functions to avoid confusion.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2ee5234) 50.69210% compared to head (340f968) 50.68798%.

Additional details and impacted files

@@                 Coverage Diff                 @@
##              master       #1453         +/-   ##
===================================================
- Coverage   50.69210%   50.68798%   -0.00413%     
===================================================
  Files            356         356                 
  Lines          24274       24274                 
  Branches         267         267                 
===================================================
- Hits           12305       12304          -1     
  Misses         10775       10775                 
- Partials        1194        1195          +1     

Flag	Coverage Δ
packages	90.62003% <ø> (ø)
solidity	93.00244% <100.00000%> (-0.08137%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ges/contracts-core/contracts/client/BaseClient.sol	100.00000% <100.00000%> (ø)
...ntracts-core/contracts/client/MessageRecipient.sol	88.88889% <100.00000%> (-11.11111%)	⬇️
...contracts-core/contracts/client/PingPongClient.sol	100.00000% <100.00000%> (ø)
...ges/contracts-core/contracts/client/TestClient.sol	100.00000% <100.00000%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.