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 1 commit
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
51 changes: 26 additions & 25 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,9 @@ 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;
parodime marked this conversation as resolved.
Show resolved Hide resolved
/// @notice Whether bridge has been relayed on destination chain
mapping(bytes32 => bool) public bridgeRelays;

Expand All @@ -43,6 +34,16 @@ 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize Getter Function Return Type

In the bridgeStatuses function, you're returning a BridgeStatus enum. While enums are convenient, returning them can be less gas-efficient. Consider returning a uint8 instead, which represents the underlying value of the enum and reduces gas consumption.

Consider this change:

-function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status)
+function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status)
{
-    return bridgeTxDetails[transactionId].status;
+    return uint8(bridgeTxDetails[transactionId].status);
}
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 bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status)
{
return bridgeTxDetails[transactionId].status;
}
function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status)
{
return uint8(bridgeTxDetails[transactionId].status);
}


function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory proof)
{
return bridgeTxDetails[transactionId].proof;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid Returning Structs Directly from Public Functions

The bridgeProofs function returns a BridgeProof struct. Returning structs can be expensive in terms of gas and may expose internal implementation details. Instead, consider returning individual fields of the struct.

Refactor the function to return individual fields:

-function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory proof)
+function bridgeProofs(bytes32 transactionId) public view returns (uint96 timestamp, address relayer)
{
-    return bridgeTxDetails[transactionId].proof;
+    BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
+    return (proof.timestamp, proof.relayer);
}

Committable suggestion was skipped due to low confidence.


constructor(address _owner) Admin(_owner) {
deployBlock = block.number;
}
Expand Down Expand Up @@ -109,7 +110,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
})
);
bytes32 transactionId = keccak256(request);
bridgeStatuses[transactionId] = BridgeStatus.REQUESTED;
bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

emit BridgeRequested(
transactionId,
Expand Down Expand Up @@ -183,9 +184,9 @@ 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[transactionId].status = BridgeStatus.RELAYER_PROVED;
bridgeTxDetails[transactionId].proof = BridgeProof({timestamp: uint96(block.timestamp), relayer: relayer}); // overflow ok
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

❗ Unexpected Status Transition Detected

The status transition from REQUESTED to REFUNDED is not defined in the expected state machine (REQUESTED → RELAYER_PROVED → RELAYER_CLAIMED). Please review the state transitions to ensure they adhere to the intended workflow.

Analysis chain

Confirm Proper Status Transition and Proof Assignment

Before updating the status to RELAYER_PROVED and assigning the proof, ensure that the transition from REQUESTED to RELAYER_PROVED is valid according to your state machine. This prevents invalid status transitions.

Run the following script to verify status transitions:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all status transitions in the code follow the defined state machine.

# Expected statuses in order: REQUESTED -> RELAYER_PROVED -> RELAYER_CLAIMED

# Search for all status updates
rg --type solidity 'bridgeTxDetails\[.*\].status = BridgeStatus\.\w+' -A 2

Length of output: 109

Script:

#!/bin/bash
# Description: Verify that all status transitions in the Solidity code follow the defined state machine.

# Expected statuses in order: REQUESTED -> RELAYER_PROVED -> RELAYER_CLAIMED

# Search for all status updates in .sol files
rg --glob '*.sol' 'bridgeTxDetails\[.*\].status\s*=\s*BridgeStatus\.\w+' -A 2

Length of output: 1552


emit BridgeProofProvided(transactionId, relayer, destTxHash);
}
Expand All @@ -204,8 +205,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {

/// @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 (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for Uninitialized Proof Before Access

When accessing bridgeTxDetails[transactionId].proof, ensure that the proof exists to prevent accessing uninitialized memory, which could lead to incorrect data retrieval.

Add a check for proof initialization:

 BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
+if (proof.relayer == address(0)) revert ProofNotInitialized();
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
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
if (proof.relayer == address(0)) revert ProofNotInitialized();

if (proof.relayer != relayer) revert SenderIncorrect();
return _timeSince(proof) > DISPUTE_PERIOD;
}
Expand All @@ -221,9 +222,9 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {
BridgeTransaction memory transaction = getBridgeTransaction(request);

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

BridgeProof memory proof = bridgeProofs[transactionId];
BridgeProof memory proof = bridgeTxDetails[transactionId].proof;

// if "to" is zero addr, permissionlessly send funds to proven relayer
if (to == address(0)) {
Expand All @@ -234,7 +235,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {

if (_timeSince(proof) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed();

bridgeStatuses[transactionId] = BridgeStatus.RELAYER_CLAIMED;
bridgeTxDetails[transactionId].status = BridgeStatus.RELAYER_CLAIMED;
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

// update protocol fees if origin fee amount exists
if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount;
Expand All @@ -249,12 +250,12 @@ contract FastBridgeV2 is Admin, IFastBridgeV2 {

/// @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();
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect();
if (_timeSince(bridgeTxDetails[transactionId].proof) > DISPUTE_PERIOD) revert DisputePeriodPassed();

// @dev relayer gets slashed effectively if dest relay has gone thru
bridgeStatuses[transactionId] = BridgeStatus.REQUESTED;
delete bridgeProofs[transactionId];
bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;
delete bridgeTxDetails[transactionId].proof;

emit BridgeProofDisputed(transactionId, msg.sender);
}
Expand All @@ -273,8 +274,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 (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect();
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED;
parodime marked this conversation as resolved.
Show resolved Hide resolved

// transfer origin collateral back to original sender
address to = transaction.originSender;
Expand Down
29 changes: 29 additions & 0 deletions packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,33 @@ 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
}
parodime marked this conversation as resolved.
Show resolved Hide resolved

struct BridgeTxDetails {
BridgeStatus status;
BridgeProof proof;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure BridgeProof type is properly defined or imported

The BridgeProof type used in the BridgeTxDetails struct is not defined within this interface. Please verify that BridgeProof is properly defined or imported from another contract or interface to prevent compilation errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not have a desired effect of saving the gas on reads and writes, as the internal composition of the struct takes more than a single storage slot (256 bits). We don't export a gas report as of now, but in this case it's possible to generate using forge t --mc FastBridgeV2SrcTest --nmt revert --gas-report.

  • Using FastBridgeV2SrcTest test contract gives us the overview for the source chain-related functions
  • --nmt revert excludes test functions having a word "revert" in the name, which makes the report more useful (revert cases in general consume less gas, and thus need to be filtered)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This are the results when run on master, prior to this PR
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

And this are the results after this PR changes:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

To gain the performance in gas consumption, the BridgeTxDetails needs to fit within 256 bits:

  • BridgeStatus enum is equivalent to uint8
  • address relayer takes 160 bits
  • which leaves up to 88 bits for the proof's timestamp

This is an opportunity to follow the suit of #3170 and also store the block number, when the prove happened. This could be useful for some of the Relayer implementations. This will require using up to 88 bits for both block number and timestamp, which is more than enough for practical purposes.



/// @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 (uint96, address);
}
Loading
Loading