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

status/proof mapping packing [SLT-186] #3173

Merged
merged 13 commits into from
Sep 27, 2024
Merged

status/proof mapping packing [SLT-186] #3173

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1348e63
status/proof mapping packing [SLT-186]
parodime Sep 23, 2024
1c5dd7d
fix BridgeProofs def to use struct
parodime Sep 23, 2024
5045835
WIP - evaluating ProofDetail struct approach
parodime Sep 25, 2024
7f5c260
fix: overflow in parity tests
ChiTimesChi Sep 25, 2024
dc2dab5
refactor BridgeTxDetails w/o addtl ProofDetail struct
parodime Sep 25, 2024
048a66f
Merge branch 'master' into feat/FbV2-storagePacking
Sep 27, 2024
ddc1f1f
master merge conflicts
Sep 27, 2024
582941f
restore bad lint warning
Sep 27, 2024
ef3d1f7
switch to fully qualified bridgeTxDetails instead of storage refs
Sep 27, 2024
2f64c94
rearrange for lint (120 char line)
Sep 27, 2024
488e378
Merge branch 'master' into feat/FbV2-storagePacking
parodime Sep 27, 2024
fbd7043
test fixes
parodime Sep 27, 2024
78c84a8
code cleanup
parodime Sep 27, 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
86 changes: 47 additions & 39 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
/// @notice Minimum deadline period to relay a requested bridge transaction
uint256 public constant MIN_DEADLINE_PERIOD = 30 minutes;

enum BridgeStatus {
NULL, // doesn't exist yet
REQUESTED,
RELAYER_PROVED,
RELAYER_CLAIMED,
REFUNDED
}

/// @notice Status of the bridge tx on origin chain
mapping(bytes32 => BridgeStatus) public bridgeStatuses;
/// @notice Proof of relayed bridge tx on origin chain
mapping(bytes32 => BridgeProof) public bridgeProofs;
mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails;
/// @notice Whether bridge has been relayed on destination chain
mapping(bytes32 => bool) public bridgeRelays;

Expand All @@ -43,6 +33,15 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
// @dev the block the contract was deployed at
uint256 public immutable deployBlock;

function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) {
return bridgeTxDetails[transactionId].status;
}

function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) {
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider returning individual values instead of BridgeProof struct

The bridgeProofs function returns a BridgeProof struct. Returning structs can be gas-inefficient and may expose internal implementation details. Consider returning individual fields of the struct instead.

-function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) {
+function bridgeProofs(bytes32 transactionId) public view returns (uint40 timestamp, address relayer) {
     BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
-    return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
+    return (_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
}

This change would maintain backwards compatibility while potentially reducing gas costs and limiting exposure of internal structures.

📝 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
function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) {
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
}
function bridgeProofs(bytes32 transactionId) public view returns (uint40 timestamp, address relayer) {
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
return (_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
}


constructor(address _owner) Admin(_owner) {
deployBlock = block.number;
}
Expand Down Expand Up @@ -109,7 +108,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
})
);
bytes32 transactionId = keccak256(request);
bridgeStatuses[transactionId] = BridgeStatus.REQUESTED;
bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;

emit BridgeRequested(
transactionId,
Expand Down Expand Up @@ -183,31 +182,34 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
function prove(bytes memory request, bytes32 destTxHash, address relayer) public onlyRole(RELAYER_ROLE) {
bytes32 transactionId = keccak256(request);
// update bridge tx status given proof provided
if (bridgeStatuses[transactionId] != BridgeStatus.REQUESTED) revert StatusIncorrect();
bridgeStatuses[transactionId] = BridgeStatus.RELAYER_PROVED;
bridgeProofs[transactionId] = BridgeProof({timestamp: uint96(block.timestamp), relayer: relayer}); // overflow ok
if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect();
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
_thisBridgeTxDetails.status = BridgeStatus.RELAYER_PROVED;
_thisBridgeTxDetails.proofBlockTimestamp = uint40(block.timestamp);
_thisBridgeTxDetails.proofBlockNumber = uint48(block.number);
_thisBridgeTxDetails.proofRelayer = relayer;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Emit event for status change to RELAYER_PROVED

After updating the status to RELAYER_PROVED, consider emitting an event to log this state change. This aids in transparency and off-chain tracking of transaction statuses.

Add an event emission:

 _thisBridgeTxDetails.status = BridgeStatus.RELAYER_PROVED;
 _thisBridgeTxDetails.proofBlockTimestamp = uint40(block.timestamp);
 _thisBridgeTxDetails.proofBlockNumber = uint48(block.number);
 _thisBridgeTxDetails.proofRelayer = relayer;
+emit BridgeStatusUpdated(transactionId, BridgeStatus.RELAYER_PROVED);

Don't forget to define the event:

event BridgeStatusUpdated(bytes32 indexed transactionId, BridgeStatus status);

This addition would improve the contract's observability and make it easier to track the lifecycle of bridge transactions.


emit BridgeProofProvided(transactionId, relayer, destTxHash);
}

/// @notice Calculates time since proof submitted
/// @dev proof.timestamp stores casted uint96(block.timestamp) block timestamps for gas optimization
/// _timeSince(proof) can accomodate rollover case when block.timestamp > type(uint96).max but
/// proof.timestamp < type(uint96).max via unchecked statement
/// @param proof The bridge proof
/// @dev proof.timestamp stores casted uint40(block.timestamp) block timestamps for gas optimization
/// _timeSince(proof) can accomodate rollover case when block.timestamp > type(uint40).max but
/// proof.timestamp < type(uint40).max via unchecked statement
/// @param proofBlockTimestamp The bridge proof block timestamp
/// @return delta Time delta since proof submitted
function _timeSince(BridgeProof memory proof) internal view returns (uint256 delta) {
function _timeSince(uint40 proofBlockTimestamp) internal view returns (uint256 delta) {
unchecked {
delta = uint96(block.timestamp) - proof.timestamp;
delta = uint40(block.timestamp) - proofBlockTimestamp;
}
}

/// @inheritdoc IFastBridge
function canClaim(bytes32 transactionId, address relayer) external view returns (bool) {
if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
BridgeProof memory proof = bridgeProofs[transactionId];
if (proof.relayer != relayer) revert SenderIncorrect();
return _timeSince(proof) > DISPUTE_PERIOD;
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
if (_thisBridgeTxDetails.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
if (_thisBridgeTxDetails.proofRelayer != relayer) revert SenderIncorrect();
return _timeSince(_thisBridgeTxDetails.proofBlockTimestamp) > DISPUTE_PERIOD;
}

/// @inheritdoc IFastBridgeV2
Expand All @@ -220,21 +222,21 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
bytes32 transactionId = keccak256(request);
BridgeTransaction memory transaction = getBridgeTransaction(request);

// update bridge tx status if able to claim origin collateral
if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];

BridgeProof memory proof = bridgeProofs[transactionId];
// update bridge tx status if able to claim origin collateral
if (_thisBridgeTxDetails.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();

// if "to" is zero addr, permissionlessly send funds to proven relayer
if (to == address(0)) {
to = proof.relayer;
} else if (proof.relayer != msg.sender) {
to = _thisBridgeTxDetails.proofRelayer;
} else if (_thisBridgeTxDetails.proofRelayer != msg.sender) {
revert SenderIncorrect();
}

if (_timeSince(proof) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed();
if (_timeSince(_thisBridgeTxDetails.proofBlockTimestamp) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed();

bridgeStatuses[transactionId] = BridgeStatus.RELAYER_CLAIMED;
_thisBridgeTxDetails.status = BridgeStatus.RELAYER_CLAIMED;

// update protocol fees if origin fee amount exists
if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount;
Expand All @@ -244,17 +246,21 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
uint256 amount = transaction.originAmount;
token.universalTransfer(to, amount);

emit BridgeDepositClaimed(transactionId, proof.relayer, to, token, amount);
emit BridgeDepositClaimed(transactionId, _thisBridgeTxDetails.proofRelayer, to, token, amount);
}

/// @inheritdoc IFastBridge
function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) {
if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
if (_timeSince(bridgeProofs[transactionId]) > DISPUTE_PERIOD) revert DisputePeriodPassed();
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];

if (_thisBridgeTxDetails.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
if (_timeSince(_thisBridgeTxDetails.proofBlockTimestamp) > DISPUTE_PERIOD) revert DisputePeriodPassed();

// @dev relayer gets slashed effectively if dest relay has gone thru
bridgeStatuses[transactionId] = BridgeStatus.REQUESTED;
delete bridgeProofs[transactionId];
_thisBridgeTxDetails.status = BridgeStatus.REQUESTED;
_thisBridgeTxDetails.proofRelayer = address(0);
_thisBridgeTxDetails.proofBlockTimestamp = 0;
_thisBridgeTxDetails.proofBlockNumber = 0;

emit BridgeProofDisputed(transactionId, msg.sender);
}
Expand All @@ -264,6 +270,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
bytes32 transactionId = keccak256(request);
BridgeTransaction memory transaction = getBridgeTransaction(request);

BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];

if (hasRole(REFUNDER_ROLE, msg.sender)) {
// Refunder can refund if deadline has passed
if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded();
Expand All @@ -273,8 +281,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
}

// set status to refunded if still in requested state
if (bridgeStatuses[transactionId] != BridgeStatus.REQUESTED) revert StatusIncorrect();
bridgeStatuses[transactionId] = BridgeStatus.REFUNDED;
if (_thisBridgeTxDetails.status != BridgeStatus.REQUESTED) revert StatusIncorrect();
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED;

// transfer origin collateral back to original sender
address to = transaction.originSender;
Expand Down
25 changes: 24 additions & 1 deletion packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ pragma solidity ^0.8.20;
import {IFastBridge} from "./IFastBridge.sol";

interface IFastBridgeV2 is IFastBridge {

/// @notice Relays destination side of bridge transaction by off-chain relayer
/// @param request The encoded bridge transaction to relay on destination chain
/// @param relayer The address of the relaying entity which should have control of the origin funds when claimed
Expand All @@ -20,4 +19,28 @@ interface IFastBridgeV2 is IFastBridge {
/// @param request The encoded bridge transaction to claim on origin chain
function claim(bytes memory request) external;

enum BridgeStatus {
NULL, // doesn't exist yet
REQUESTED,
RELAYER_PROVED,
RELAYER_CLAIMED,
REFUNDED
}

struct BridgeTxDetails {
BridgeStatus status;
uint40 proofBlockTimestamp;
uint48 proofBlockNumber;
address proofRelayer;
}

/// @notice Returns the status of a bridge transaction
/// @param transactionId The ID of the bridge transaction
/// @return The status of the bridge transaction
function bridgeStatuses(bytes32 transactionId) external view returns (BridgeStatus);

/// @notice Returns the timestamp and relayer of a bridge proof
/// @param transactionId The ID of the bridge transaction
/// @return The timestamp and relayer address of the bridge proof
function bridgeProofs(bytes32 transactionId) external view returns (BridgeProof memory);
}
34 changes: 19 additions & 15 deletions packages/contracts-rfq/test/FastBridge.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
// solhint-disable

import "forge-std/Test.sol";
import "forge-std/console2.sol";
Expand Down Expand Up @@ -51,6 +52,19 @@ contract FastBridgeTest is Test {
ethUSDC.mint(dstUser, 100 * 10 ** 6);
}

function assertCorrectProof(
bytes32 transactionId,
uint256 expectedTimestamp,
address expectedRelayer
)
internal
virtual
{
(uint96 timestamp, address relayer) = fastBridge.bridgeProofs(transactionId);
assertEq(timestamp, uint96(expectedTimestamp));
assertEq(relayer, expectedRelayer);
}

function _getBridgeRequestAndId(
uint256 chainId,
uint256 currentNonce,
Expand Down Expand Up @@ -1349,9 +1363,7 @@ contract FastBridgeTest is Test {
fastBridge.prove(request, fakeTxnHash);

// We check if the bridge transaction proof timestamp is set to the timestamp at which the proof was provided
(uint96 _timestamp, address _oldRelayer) = fastBridge.bridgeProofs(transactionId);
assertEq(_timestamp, uint96(block.timestamp));
assertEq(_oldRelayer, relayer);
assertCorrectProof(transactionId, block.timestamp, relayer);

// We check if the bridge status is RELAYER_PROVED
assertEq(uint256(fastBridge.bridgeStatuses(transactionId)), uint256(FastBridge.BridgeStatus.RELAYER_PROVED));
Expand Down Expand Up @@ -1383,9 +1395,7 @@ contract FastBridgeTest is Test {
fastBridge.prove(request, fakeTxnHash);

// We check if the bridge transaction proof timestamp is set to the timestamp at which the proof was provided
(uint96 _timestamp, address _oldRelayer) = fastBridge.bridgeProofs(transactionId);
assertEq(_timestamp, uint96(block.timestamp));
assertEq(_oldRelayer, relayer);
assertCorrectProof(transactionId, block.timestamp, relayer);

// We stop a prank to contain within test
vm.stopPrank();
Expand Down Expand Up @@ -1414,9 +1424,7 @@ contract FastBridgeTest is Test {
fastBridge.prove(request, fakeTxnHash);

// We check if the bridge transaction proof timestamp is set to the timestamp at which the proof was provided
(uint96 _timestamp, address _oldRelayer) = fastBridge.bridgeProofs(transactionId);
assertEq(_timestamp, uint96(block.timestamp));
assertEq(_oldRelayer, relayer);
assertCorrectProof(transactionId, block.timestamp, relayer);

// We stop a prank to contain within test
vm.stopPrank();
Expand Down Expand Up @@ -1713,10 +1721,8 @@ contract FastBridgeTest is Test {
fastBridge.dispute(transactionId);

// check status and proofs updated
(uint96 _timestamp, address _oldRelayer) = fastBridge.bridgeProofs(transactionId);
assertEq(uint256(fastBridge.bridgeStatuses(transactionId)), uint256(FastBridge.BridgeStatus.REQUESTED));
assertEq(_timestamp, 0);
assertEq(_oldRelayer, address(0));
assertCorrectProof(transactionId, 0, address(0));

// We stop a prank to contain within test
vm.stopPrank();
Expand All @@ -1739,10 +1745,8 @@ contract FastBridgeTest is Test {
fastBridge.dispute(transactionId);

// check status and proofs updated
(uint96 _timestamp, address _oldRelayer) = fastBridge.bridgeProofs(transactionId);
assertEq(uint256(fastBridge.bridgeStatuses(transactionId)), uint256(FastBridge.BridgeStatus.REQUESTED));
assertEq(_timestamp, 0);
assertEq(_oldRelayer, address(0));
assertCorrectProof(transactionId, 0, address(0));

// We stop a prank to contain within test
vm.stopPrank();
Expand Down
15 changes: 15 additions & 0 deletions packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,21 @@ contract FastBridgeV2ParityTest is FastBridgeTest {
return deployCode({what: "FastBridgeV2", args: abi.encode(owner)});
}

/// @notice We use uint40 for the timestamps in FastBridgeV2
function assertCorrectProof(
bytes32 transactionId,
uint256 expectedTimestamp,
address expectedRelayer
)
internal
virtual
override
{
(uint96 timestamp, address relayer) = fastBridge.bridgeProofs(transactionId);
assertEq(timestamp, uint40(expectedTimestamp));
assertEq(relayer, expectedRelayer);
}

/// @notice Relay function is no longer permissioned, so we skip this test
function test_failedRelayNotRelayer() public virtual override {
vm.skip(true);
Expand Down
Loading
Loading