Skip to content

Update Registry.sol#289

Merged
thelostone-mc merged 1 commit intoallo-protocol:mainfrom
0xScratch:patch-6
Sep 1, 2023
Merged

Update Registry.sol#289
thelostone-mc merged 1 commit intoallo-protocol:mainfrom
0xScratch:patch-6

Conversation

@0xScratch
Copy link
Contributor

Three types of changes made in this PR:

1st -> changing uint256 i = 0 to uint256 i, explained in #288

2nd -> Making an internal function for the modifier onlyProfileOwner which kind of appeared 5-6 times in that particular file..
Explaination: Usually modifiers just copy paste their code whenever they are used in any function, thus costs some amount of gas, but creating an internal function and aligning it with our modifier really drops down the gas usage at the cost of JUMP opcode

3rd -> Just some typos and grammatical errors in the comments, usually a change of an member to a member

if (!_isOwnerOfProfile(_profileId, msg.sender)) {
revert UNAUTHORIZED();
}
_checkOnlyProfileOwner(_profileId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to do this, it is only used in this one place, so you are still following SOLID principles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I was late in explaining and showing the result of the change we can get, but anyways

Yeah, I know that internal function was used at just one place, but it's related to modifier which is kind of used up several times. Try to run some tests and we be getting a clear scenario.

/// ======== Internal Functions ========
/// ====================================

/// @dev Internal function used by modifier 'onlyProfileOwner'
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xScratch
Copy link
Contributor Author

Quite a demo related to 2nd change (Modifiers one)

So I created a similar basic look code, which have two modifiers onlyBuyer and onlySeller and they are being used in 3 and 5 functions respectively (just for mimicking the original one). Here's the code:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract Demo{
    address public seller;
    address public buyer;

    error notBuyer();
    error notSeller();

    modifier onlyBuyer(address _buyer){
        if(_buyer != buyer) revert notBuyer();
        _;
    }
    modifier onlySeller(address _seller){
        if(_seller != seller) revert notSeller();
        _;
    }

    function Buyer1(address buyer_) external onlyBuyer(buyer_){
        // some code
    }

    function Buyer2(address buyer_) external onlyBuyer(buyer_){
        // some code
    }

    function Buyer3(address buyer_) external onlyBuyer(buyer_){
        // some code
    }

    function Seller1(address seller_) external onlySeller(seller_){
        // some code
    }

    function Seller2(address seller_) external onlySeller(seller_){
        // some code
    }

    function Seller3(address seller_) external onlySeller(seller_){
        // some code
    }

    function Seller4(address seller_) external onlySeller(seller_){
        // some code
    }

    function Seller5(address seller_) external onlySeller(seller_){
        // some code
    }
}

At first, I ran this code with simple modifiers like we usually use and clicked a snapshot of the amount of gas it used:

Screenshot (22)

and then after making the required changes like I have done in this PR, or just see the code itself (the changed part):

    modifier onlyBuyer(address _buyer){
        checkBuyer(_buyer);
        _;
    }
    modifier onlySeller(address _seller){
        checkSeller(_seller);
        _;
    }

    function checkBuyer(address _buyer) internal view{
        if (_buyer != buyer) revert notBuyer();
    }

    function checkSeller(address _seller) internal view{
        if (_seller != seller) revert notSeller();
    }

I got these results which were really striking!!

Screenshot (23)

That's it, Thanks for making a through reading!!

@thelostone-mc
Copy link
Member

thelostone-mc commented Sep 1, 2023

Before:
image

After:
image

Rebasing against main and running formatter

thelostone-mc
thelostone-mc previously approved these changes Sep 1, 2023
@thelostone-mc thelostone-mc merged commit 8c5f5f8 into allo-protocol:main Sep 1, 2023
@0xScratch 0xScratch deleted the patch-6 branch September 1, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants