Skip to content

⚡️ STL Optimizations#259

Open
Vectorized wants to merge 16 commits intotransmissions11:v7from
Vectorized:play2
Open

⚡️ STL Optimizations#259
Vectorized wants to merge 16 commits intotransmissions11:v7from
Vectorized:play2

Conversation

@Vectorized
Copy link
Copy Markdown
Contributor

@Vectorized Vectorized commented May 30, 2022

Description

Changes:

You can try copypasta the revert assembly into something like Remix and verify it.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

@Vectorized Vectorized closed this May 30, 2022
@Vectorized Vectorized reopened this May 30, 2022
@z0r0z
Copy link
Copy Markdown
Contributor

z0r0z commented May 30, 2022

I like it. If we can't get t11s to accept custom errors, let's use assemblored reverts.

@transmissions11
Copy link
Copy Markdown
Owner

transmissions11 commented May 30, 2022

I like it. If we can't get t11s to accept custom errors, let's use assemblored reverts.

lol i opened the issue to do reverts in asm #157

@z0r0z
Copy link
Copy Markdown
Contributor

z0r0z commented May 30, 2022

ok ok ok

@Vectorized
Copy link
Copy Markdown
Contributor Author

Vectorized commented Jun 2, 2022

The test fails for ERC1155Test::testFailFuzzSafeBatchTransferInsufficientBalance for certain rare fuzz cases.

You can try the following:

function testFailFuzzSafeBatchTransferInsufficientBalanceRareCase() public {
    uint256[] memory ids = new uint256[](2);
    ids[0] = 0;
    ids[1] = 0;
    uint256[] memory mintAmounts = new uint256[](2);
    mintAmounts[0] = type(uint256).max - 2;
    mintAmounts[1] = 2;
    uint256[] memory transferAmounts = new uint256[](2);
    transferAmounts[0] = type(uint256).max - 2;
    transferAmounts[1] = 2;
    testFailFuzzSafeBatchTransferInsufficientBalance(
        address(0xABCD),
        ids, 
        mintAmounts,
        transferAmounts, 
        bytes(""), 
        bytes("")
    );
}

The fix is to simply add a continue line in the fuzz test like so:

for (uint256 i = 0; i < minLength; i++) {
    uint256 id = ids[i];

    uint256 remainingMintAmountForId = type(uint256).max - userMintAmounts[from][id];

    if (mintAmounts[i] == remainingMintAmountForId) continue; // To mitigate the rare case.

    uint256 mintAmount = bound(mintAmounts[i], 0, remainingMintAmountForId);
    uint256 transferAmount = bound(transferAmounts[i], mintAmount + 1, type(uint256).max);

    normalizedIds[i] = id;
    normalizedMintAmounts[i] = mintAmount;
    normalizedTransferAmounts[i] = transferAmount;

    userMintAmounts[from][id] += mintAmount;
}

Lemme know if you want to add this line into the fuzz test, although it is not related to the PR.

@Vectorized
Copy link
Copy Markdown
Contributor Author

Note that we need 4 mstores for the reverts, as the scratch space may be dirty from those special tricks.

@Vectorized Vectorized changed the title ⚡️ Assembly reverts for STL ⚡️ STL Optimizations Jun 22, 2022
and(
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(eq(mload(0x00), 1), iszero(returndatasize())),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

wait how is it safe to remove the gt(returndatasize(), 31)) check

Copy link
Copy Markdown
Contributor Author

@Vectorized Vectorized Jun 22, 2022

Choose a reason for hiding this comment

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

Before the call, we store the 4-byte selector at 0x00.

// Equivalent to `mstore(0x00, 0x0000000000000000000000000000000000000000000000000000000023b872dd)`
mstore(0x00, 0x23b872dd)

Since the selector 0x23b872dd does not end with byte 0x01
(e.g. 0x11223301)…

the only way for the memory slot at 0x00 to be equal to 1 is for the call to write 0x0000000000000000000000000000000000000000000000000000000000000001 to 0x00, which gives a returndatasize() that is at least 32.

This can be applied for the other selectors too (0xa9059cbb, 0x095ea7b3).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

oh what the fuck this is genius

@sambacha
Copy link
Copy Markdown
Contributor

sambacha commented Jul 4, 2022

This is unreadable

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.

4 participants