tokamak-network / tokamak-bridge

An interface designed to bridge assets between Titan and Ethereum chains.
https://app.bridge.tokamak.network
6 stars 0 forks source link

[Cross Trade - Suggestion]: Remove _msgSender parameter from cancelCT method #203

Closed blackcow1987 closed 4 weeks ago

blackcow1987 commented 1 month ago

What is your recommendation for?

No response

Explain your suggestion

In the cancelCT method, _msgSender is equal to dealData.requester, and since cancelCT can only be called from an L1 contract, it is much cheaper to read the value stored in dealData from L2 than to pass it as calldata.

monica-tokamak commented 1 month ago

@zzooppii

zzooppii commented 1 month ago

I'm still using the values ​​saved in dealData. I think all of the _msgSender, _salecount, _chainId, and _hash values ​​are needed when executing Cancel in L2 when sending to L2.

Could you please show me the code for how to do it? I think the current configuration is correct.

blackcow1987 commented 1 month ago
    function cancelCT(
        address _msgSender,
        uint256 _salecount,
        uint256 _chainId,
        bytes32 _hash
    )
        external
        nonReentrant
        checkL1(_chainId)
        providerCheck(_salecount)
    {
        require(dealData[_salecount].requester == _msgSender, "your not seller");

This is piece of cancelCT method in L2CrossTrade.sol. At this point, user can't fake _saleCount / _msgSender / _chainId because this value is already applied to the _hash calculated in the cancel method of L1CrossTrade. In my analysis, cancelCT method can be modified like this:

function cancelCT(bytes32 _hash) {
    ...
    RequestData memory req = dealData[_hash];
     ...
zzooppii commented 1 month ago

I understand. Right now, the unique value is saleCount, but you are saying that we can proceed by changing it to hash.

We will discuss the relevant part.

Thank you for the good feedback!

zzooppii commented 4 weeks ago

@blackcow1987 The overall storage structure has changed, so it's difficult this time.

I'll apply it next time.

Thank you.

zzooppii commented 1 week ago

Duplicate of #200