-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add idToMarket getter #278
Conversation
I don't think the suggestion was to put this in the main contract. From what I understand, the suggestion is to put this in a periphery, and would be a good fit for the bulker contract. |
The issue with putting it in the periphery is that nothing can enforce that it is stored at the market creation. That's why I'm proposing to add it here. In any case, the feasibility and gas improvement of such a thing must be tested first. |
I question whether there is even a need for it to be stored at market creation. If we assume that our hashing scheme is cryptographically secure (which I believe is a safe assumption), then any contract can verify and store this information without it being inside the core contract. Which I believe is exactly what is being suggested by @ncitron |
Agree that he's suggesting to add an external contract, but that would require some management while here there's not management at all for a relatively small gas cost. |
Though there is some management, the management would basically be permissionless. Anyone should be able to submit a market that hashes to the Id, and then the market can be cached inside storage. If I understand correctly, the main contract cannot even use this storage variable at all. It is only useful for a periphery, and only on a rollup where calldata optimization is prioritized. Therefore I think it should be managed by a periphery. |
Good point. I find it quite annoying though, because you lose some synchronicity. I mean you can't build a contract that assumes that the mapping is correct. |
There is a cryptographic guarantee that the mapping is either correct, or that it is uninitialized though. It should not be possible to find two market structs that hash to the same Id. In the case that it is uninitialized, the getter can simply revert with an error that signals to the user that the slot must be initialized with a market struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some naming suggestions
Note that for this PR we must compute what is the gas cost of passing market as input VS retrieving it onchain (through a bulker for instance) before interacting on blue on a L2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned here, if we're adding this mapping I think we should also add a list of the ids of the created markets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #208