Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FbV2 sender nonces [SLT-183] #3214

Merged
merged 4 commits into from
Oct 2, 2024
Merged

FbV2 sender nonces [SLT-183] #3214

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a913eb
sender nonces [SLT-183]
parodime Oct 2, 2024
15ca0de
refactor nonce test for codecov
parodime Oct 2, 2024
52da969
refactor: make `nonce` an immutable var
ChiTimesChi Oct 2, 2024
0cb26c4
chore: ignore local coverage report
ChiTimesChi Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@
mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails;
/// @notice Relay details on destination chain
mapping(bytes32 => BridgeRelay) public bridgeRelayDetails;

/// @dev to prevent replays
uint256 public nonce;
/// @notice Unique bridge nonces tracked per originSender
mapping(address => uint256) public senderNonces;

// @dev the block the contract was deployed at
uint256 public immutable deployBlock;
Expand Down Expand Up @@ -113,6 +112,12 @@
return _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD;
}

/// @notice This function is deprecated and should not be used.
/// @dev Replaced by senderNonces
function nonce() external pure returns (uint256) {
return 0;

Check warning on line 118 in packages/contracts-rfq/contracts/FastBridgeV2.sol

View check run for this annotation

Codecov / codecov/patch

packages/contracts-rfq/contracts/FastBridgeV2.sol#L118 

Added line #L118 was not covered by tests
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider modifying or removing the deprecated nonce() function

The nonce() function is deprecated and currently returns 0, which might lead to confusion or unintended behavior if external callers rely on its previous functionality. Consider either removing the function or updating it to revert with a clear deprecation message to prevent misuse.

Suggested change:

function nonce() external pure returns (uint256) {
-    return 0;
+    revert("nonce() is deprecated and should not be used.");
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// @notice This function is deprecated and should not be used.
/// @dev Replaced by senderNonces
function nonce() external pure returns (uint256) {
return 0;
}
/// @notice This function is deprecated and should not be used.
/// @dev Replaced by senderNonces
function nonce() external pure returns (uint256) {
revert("nonce() is deprecated and should not be used.");
}

/// @inheritdoc IFastBridge
function getBridgeTransaction(bytes memory request) external pure returns (BridgeTransaction memory) {
// Note: when passing V2 request, this will decode the V1 fields correctly since the new fields were
Expand Down Expand Up @@ -158,7 +163,7 @@
originFeeAmount: originFeeAmount,
sendChainGas: params.sendChainGas,
deadline: params.deadline,
nonce: nonce++, // increment nonce on every bridge
nonce: senderNonces[params.sender]++, // increment nonce on every bridge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential off-by-one error due to postfix increment operator

The use of the postfix increment operator senderNonces[params.sender]++ assigns the current nonce value and then increments it. This means the nonce included in the request is the pre-incremented value. If the intention is to include the incremented nonce value, consider using the prefix increment operator ++senderNonces[params.sender] to avoid potential off-by-one errors.

Suggested change:

-                    nonce: senderNonces[params.sender]++, // increment nonce on every bridge
+                    nonce: ++senderNonces[params.sender], // increment nonce on every bridge
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
nonce: senderNonces[params.sender]++, // increment nonce on every bridge
nonce: ++senderNonces[params.sender], // increment nonce on every bridge

exclusivityRelayer: paramsV2.quoteRelayer,
// We checked exclusivityEndTime to be in range (0 .. params.deadline] above, so can safely cast
exclusivityEndTime: uint256(exclusivityEndTime)
Expand Down
5 changes: 5 additions & 0 deletions packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ abstract contract FastBridgeV2SrcBaseTest is FastBridgeV2Test {
fastBridge.refund(abi.encode(bridgeTx));
}

function test_nonce() public view {
// deprecated. should always return zero in FbV2.
assertEq(fastBridge.nonce(), 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Update the test_nonce() function to reflect the new per-sender nonce implementation

The FastBridgeV2.sol contract now implements per-sender nonces using the senderNonces mapping. However, the test_nonce() function in FastBridgeV2.Src.Base.t.sol asserts that fastBridge.nonce() is always zero and is marked as deprecated. This discrepancy can lead to inaccurate test coverage and potential misunderstandings of the contract's functionality.

  • Action Items:
    1. Revise the test_nonce() function to test the senderNonces mapping correctly, ensuring that nonces increment per sender as intended.
    2. Remove or update the deprecated comment to clarify the current status and purpose of the test_nonce() function.
    3. Ensure all related tests are updated to align with the new nonce implementation to maintain consistent and reliable test coverage.
🔗 Analysis chain

Clarify the purpose of the test_nonce() function

The newly added test_nonce() function appears to be inconsistent with the PR objectives. While the PR aims to implement a per-sender nonce, this test function asserts that the nonce is always zero and is marked as deprecated. This raises several concerns:

  1. If the nonce is always zero, it doesn't align with the goal of implementing a per-sender nonce.
  2. The "deprecated" comment suggests this might be temporary or outdated code.
  3. The function name implies it's a test, but it's not using a testing framework assertion (e.g., assert instead of assertEq).

Consider the following actions:

  1. Clarify if this function is intended to be part of the final implementation or if it's a temporary placeholder.
  2. If it's meant to stay, update the function to properly test the new per-sender nonce behavior.
  3. If it's temporary, consider removing it or adding a TODO comment explaining its future purpose.

To better understand the context, let's check for other nonce-related implementations:

This will help us verify if the nonce implementation has been updated elsewhere in the contract.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for nonce-related implementations in the FastBridgeV2 contract
rg --type solidity -i "nonce" packages/contracts-rfq/src/FastBridgeV2.sol

Length of output: 106

Script:

#!/bin/bash
# Search for nonce-related implementations in the FastBridgeV2 contract without specifying file type
rg -i "nonce" packages/contracts-rfq/src/FastBridgeV2.sol

Length of output: 141

Script:

#!/bin/bash
# Search for FastBridgeV2.sol in the entire repository
fd "FastBridgeV2\.sol" . -t f

Length of output: 147

Script:

#!/bin/bash
# Search for nonce-related implementations in FastBridgeV2.sol
rg -i "nonce" packages/contracts-rfq/contracts/FastBridgeV2.sol

Length of output: 360


function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public pure {
assertEq(uint8(a), uint8(b));
}
Expand Down
18 changes: 17 additions & 1 deletion packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(tokenTx, txId);
expectBridgeQuoteDetails(txId, tokenParamsV2.quoteId);
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkTokenBalancesAfterBridge(userA);
}
Expand All @@ -112,6 +114,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(tokenTx, txId);
expectBridgeQuoteDetails(txId, tokenParamsV2.quoteId);
bridge({caller: userB, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
assertEq(srcToken.balanceOf(userA), LEFTOVER_BALANCE + tokenParams.originAmount);
checkTokenBalancesAfterBridge(userB);
Expand All @@ -126,29 +130,39 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
function test_bridge_eth() public {
// bridge token first to match the nonce
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
bytes32 txId = getTxId(ethTx);
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 2);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkEthBalancesAfterBridge(userA);
}

function test_bridge_eth_diffSender() public {
// bridge token first to match the nonce
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
bytes32 txId = getTxId(ethTx);
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
// bridge for user A as sender, called by userB
bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 2);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
assertEq(userA.balance, LEFTOVER_BALANCE + ethParams.originAmount);
checkEthBalancesAfterBridge(userB);
}

function test_bridge_userSpecificNonce() public {
vm.skip(true); // TODO: unskip when implemented
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
// UserB nonce is 0
ethTx.nonce = 0;
ethParams.sender = userB;
Expand All @@ -157,6 +171,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 1);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkEthBalancesAfterBridge(userB);
}
Expand Down
Loading