feat(store): add encodeKeyTuple function to tablegen
#753
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I'm pretty sure too many keys+fields always caused stack too deep |
You could add the function but not replace inlined code, the rendering function encapsulates the logic already, and the solidity code is autogenerated and code repetition doesn't matter |
+1 to this, I think we have a lot of room to optimize gas over time by just repeating code in areas of codegen |
Yes, it will always happen eventually, but now it happens on the
Fair, but the |
yes, I wasn't thinking of gas at the time. You don't have to either at this moment, we'll need separate gas optimization PRs later anyways, I just mentioned it now when I had the thought |
yeah probably some nuances of additional stack space for internal method calls. Inlining would solve it, so even if you don't wanna optimize gas here you might still wanna go for it |
I agree, I don't want to leave this error or have to modify the test example. I will inline the |
| @@ -129,6 +130,15 @@ library ${libraryName} { | |||
| ])}); | |||
| } | |||
|
|
|||
| /** Encode keys as a bytes32 array using this table's schema */ | |||
| function encodeKey(${renderArguments([_typedKeyArgs])}) internal pure returns (bytes32[] memory _primaryKeys) { | |||
There was a problem hiding this comment.
not blocking but thinking we will want to consolidate our language around "key" vs "primary key" vs "key tuple" and wondering if we wanna just go ahead and name this encodeKeyTuple here (see #760)
There was a problem hiding this comment.
Agreed - I will use encodeKeyTuple. I much prefer "key tuple" as we represent primary keys in many ways.
To be clear I don't think we should rename the _primaryKeys vars in this PR (not that you are proposing that).
yonadaaa
changed the title
yonadaaa
changed the title
encodeKeyTuple function to tablegen
holic
changed the title
encodeKeyTuple function to tablegenencodeKeyTuple function to tablegen
fixes #751
This simplifies the Table contracts, but replacing the inlined code with an internal function increases gas use :(
It also causes
stack too deeperror on worlds with too many keys + fields.