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.

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

Change tokenApproval slot calculation to avoid compiler error #364

Merged
merged 4 commits into from
Jul 11, 2022

Conversation

emo-eth
Copy link
Contributor

@emo-eth emo-eth commented Jul 10, 2022

This should mostly be a nit/cleanup thing, but I was running into errors when compiling with --via-ir in a project I'm working on.

Some combination of factors led the IR pipeline to fail, presumably when trying to put the slot pointer onto the stack outside of the assembly block:

❯ forge build
[⠆] Compiling...
[⠊] Compiling 25 files with 0.8.15
[⠒] Solc 0.8.15 finished in 1.61s
Error: 
Compiler run failed
InternalCompilerError: Invalid stack item name: slot

Referencing the slot directly in the mstore seemed to fix it on my end, and shouldn't cost any extra gas.

@Vectorized
Copy link
Collaborator

This will maintain the slot position for diamonds.

Copypasta the following into Remix and test

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0;

contract TokenApprovalTest {
    struct TokenApprovalRef {
        address value;
    }

    struct Layout {
        mapping(uint256 => TokenApprovalRef) _tokenApprovals;
    }

    bytes32 internal constant STORAGE_SLOT = keccak256('LoremIpsum');

    function layout() internal pure returns (Layout storage l) {
        bytes32 s = STORAGE_SLOT;
        assembly {
            l.slot := s
        }
    }

    function getSlot(uint256 tokenId) external view returns (uint256 result) {
        TokenApprovalRef storage tokenApproval = layout()._tokenApprovals[tokenId];
        assembly {
            result := tokenApproval.slot
        }
        // 0: 91113508023499702123240479191097072207067519098954698186524668019222587556952
        // 1: 109446771143514877113816426284965643011704570302985655154505988876143917244871
    }
}
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0;

contract TokenApprovalTest {
    struct TokenApprovalRef {
        address value;
    }

    struct Layout {
        mapping(uint256 => address) _tokenApprovals;
    }

    bytes32 internal constant STORAGE_SLOT = keccak256('LoremIpsum');

    function layout() internal pure returns (Layout storage l) {
        bytes32 s = STORAGE_SLOT;
        assembly {
            l.slot := s
        }
    }

    function getSlot(uint256 tokenId) external view returns (uint256 result) {
        mapping(uint256 => address) storage tokenApprovalsPtr = layout()._tokenApprovals;
        assembly {
            mstore(0x00, tokenId)
            mstore(0x20, tokenApprovalsPtr.slot)
            result := keccak256(0x00, 0x40)
        }
        // 0: 91113508023499702123240479191097072207067519098954698186524668019222587556952
        // 1: 109446771143514877113816426284965643011704570302985655154505988876143917244871
    }
}

@Vectorized Vectorized requested a review from cygaar July 10, 2022 21:47
@Vectorized Vectorized changed the title Use _tokenApprovals.slot directly in _getApprovedAddress Change tokenApproval slot calculation to avoid compiler error Jul 10, 2022
@Vectorized Vectorized linked an issue Jul 10, 2022 that may be closed by this pull request
@Vectorized
Copy link
Collaborator

Thanks for alerting us to this. @jameswenzel

Comment on lines +34 to +36
struct TokenApprovalRef {
address value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to mention that we could move this to the interface, but for private use, I think it's ok to be here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely only for private use.

Gave it to a more obscure name like TokenApprovalRef instead of TokenApproval to prevent namespace collision.

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.

⚠️ Invalid stack item name: slot
4 participants