add claimCompressed #238

waynehoover · 2023-12-13T02:04:19Z

Non breaking changes

Adds new claimCompressed function for optimized calldata, it just forwards to claimOptimized, so no breaking changes.
Removes the QuestCreatedWithAction event as it wasn't used.
Removes the ability to create sablier quests in the factory, as it was complicating things and it's not currently used.
Adds new createERC20Quest and createERC1155Quest functions that include two new string params: actionType_ and questName_.

Deployment plan

Because this depends on new state being saved on quest creation, i.e. questName and actionType the new claimCompressed function not work on any quests that don't have this data saved on chain, this function should only be used on quests that have this data.
When this is deployed we need to change the ABI where used when creating quests to use the new functions

Ideas for further reducing claim calldata

Create a forwarding contract that only has a fallback, to remove the need for a function selector.
use abi.encodePacked to encode the calldata and calldataload to get data from offset (not recommended if ever plan to pass dynamic types), this would remove the need for LibZip
use a lookup enum passing the chainid

Quazia

LGTM - tried to signal to anywhere I see room for future improvements but I think this is good as is. Do with my comments what you wish

Quazia · 2023-12-15T16:55:57Z

contracts/QuestFactory.sol

@@ -147,6 +148,7 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
    /// @param totalParticipants_ The total amount of participants (accounts) the quest will have
    /// @param tokenId_ The reward token id of the erc1155 at rewardTokenAddress_
    /// @param questId_ The id of the quest
+    /// @param actionType_ The action type for the quest


Add questName_ to params

Quazia · 2023-12-15T17:08:18Z

contracts/QuestFactory.sol

+        bytes memory uuid = new bytes(36); // UUID length with hyphens
+
+        uint256 j = 0; // Position in uuid
+        for (uint256 i = 0; i < 16; i++) {


I think we can do this with a single index vs 2 if we keep the j++ statements and do a while instead of a for. I think it'd be possible with a for too just without the third statement. Not sure if this is worth it though - as we've found compute is cheap!

Quazia · 2023-12-15T17:14:18Z

contracts/QuestFactory.sol

+        ));
+    }
+
+    function bytes16ToUUID(bytes16 data) public pure returns (string memory) {


Can you comment the encoding and expectations around the form of the bytes16 data here?

Quazia · 2023-12-15T17:17:11Z

contracts/QuestFactory.sol

+        (
+            address ref,
+            bytes32 txHash,
+            uint32 txHashChainId,


There's few enough chains that we could use a smaller value here and then leverage a lookup table - uint8 should work fine for.

Fine to capture this for a follow-on just trying to see where else we could trim the call data

Added notes in the description about possible ways to get the bytes down in the future.

Quazia · 2023-12-15T17:34:52Z

contracts/QuestFactory.sol

@@ -293,6 +247,68 @@ contract QuestFactory is Initializable, LegacyStorage, OwnableRoles, IQuestFacto
    /*//////////////////////////////////////////////////////////////
                                 CLAIM
    //////////////////////////////////////////////////////////////*/
+    function claimCompressed(bytes calldata compressedData) external payable {
+        bytes memory data = LibZip.cdDecompress(compressedData);


This is wild - do we know if ordering effects the efficiency of the compression at all? That code is hard to parse.

Ordering does, generally you want to have zero bytes in the middle.

Quazia · 2023-12-15T17:34:55Z

contracts/QuestFactory.sol

+            bytes16 questid,
+            bytes32 r,
+            bytes32 vs
+        ) = abi.decode(


Can we put the params smaller than a block at the end? I don't think it matters here but generally good form.

Quazia · 2023-12-15T17:36:01Z

contracts/QuestFactory.sol

+            (address, bytes32, uint32, bytes16, bytes32, bytes32)
+        );
+
+        string memory questIdString = bytes16ToUUID(questid);


So cool - love this. I'm surprised there's not an off-the-shelf bytes16toUUID util. Maybe we can ask for one in solady/solmate and import that once the optomizzeeddd version is available wdyt? Again though, not super important since apparently compute is so cheap on L2

Quazia · 2023-12-15T17:36:28Z

contracts/QuestFactory.sol

+
+        string memory questIdString = bytes16ToUUID(questid);
+        Quest storage quest = quests[questIdString];
+        string memory jsonData = buildJsonString(uint256(txHash).toHexString(), uint256(txHashChainId).toString(), quest.actionType, quest.questName);


Ah - here is where you're pulling that back in. Totally makes sense now.

Quazia · 2023-12-15T17:37:24Z

contracts/QuestFactory.sol

+        return string(uuid);
+    }
+
+
    function claimOptimized(bytes calldata signature_, bytes calldata data_) external payable {


We should change the visibility of this to internal or public if we're using it internally.

The idea is to keep it external so that it's backwards compatible. Eventually once everyone is using the new function we can make it internal.

waynehoover added 2 commits December 12, 2023 16:03

add claimOptimized

293be92

merge main

49fc445

waynehoover requested a review from a team as a code owner December 13, 2023 02:04

remove

db44281

waynehoover changed the title ~~add claimOptimized~~ add claimCompressed Dec 13, 2023

waynehoover added 10 commits December 12, 2023 16:39

wip

6bdf5e5

more wip

9fd54e3

wip

7f3fe4c

add tests

21d7f28

add more tests

533632c

Merge remote-tracking branch 'origin/main' into reduced_calldata_proxy

54d453b

add intergation test

3d64f21

add env var

be7fcd1

update .env.example

2686836

env var rename

8afdd98

waynehoover requested a review from Quazia December 15, 2023 02:13

Quazia approved these changes Dec 15, 2023

View reviewed changes

waynehoover added 6 commits December 15, 2023 09:58

make quest creation backwards compat

fc765a3

CR feedback updates

9252ac0

be consistant with _

1ba8fa9

deploy to sepolia

b96e197

deploy everywhere

eae79d0

deploy everywhere

ea8b612

waynehoover merged commit e03e6ce into main Dec 19, 2023
2 checks passed

waynehoover deleted the reduced_calldata_proxy branch December 19, 2023 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add claimCompressed #238

add claimCompressed #238

waynehoover commented Dec 13, 2023 •

edited

Loading

Quazia left a comment

Quazia Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

waynehoover Dec 15, 2023

Quazia Dec 15, 2023

waynehoover Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

Quazia Dec 15, 2023

waynehoover Dec 15, 2023

add claimCompressed #238

add claimCompressed #238

Conversation

waynehoover commented Dec 13, 2023 • edited Loading

Non breaking changes

Deployment plan

Ideas for further reducing claim calldata

Quazia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynehoover commented Dec 13, 2023 •

edited

Loading