put market in memory instead of calldata

[MerlinEgalite]

Fixes #144

Few questions:

• Should we move these new functions to an helper library instead?
• Should we rename id to idCalldata or something similar?

[Rubilmax]

@MathisGD suggested to actually move from calldata to memory: #144 (comment)

But for completeness, we'd have to expose a calldata-based getter anyway, right?

On a side note, this PR looks ridiculous but we'll actually use such function/library in the test suite - perhaps we could include some change to illustrate it.

[MerlinEgalite]

@MathisGD suggested to actually move from calldata to memory: #144 (comment)

But calldata is actually cheaper so I don't think we should this tbh.

But for completeness, we'd have to expose a calldata-based getter anyway, right?

Yes.

On a side note, this PR looks ridiculous but we'll actually use such function/library in the test suite - perhaps we could include some change to illustrate it.

Yes I'll adapt

[makcandrov]

So bad that Solidity doesn't allow giving the same name to all the functions...

Should we move these new functions to an helper library instead?

I find it more elegant to keep them in the same library. Also, we might need them in Blue at some point.

Should we rename id to idCalldata or something similar?

From the library's perspective, I find it more elegant to remain consistent and always specify the variable location. But it does burden Blue's code... Maybe we can shorten the names? Something like idc, idm, & ids?

But calldata is actually cheaper so I don't think we should this tbh.

I agree that we should keep calldata.

[MerlinEgalite]

From the library's perspective, I find it more elegant to remain consistent and always specify the variable location. But it does burden Blue's code... Maybe we can shorten the names? Something like idc, idm, & ids?

Can be better yers but I think ids is very misleading though

[Rubilmax]

mId, sId, cId? memId, stgId, cdtId?

[makcandrov]

mId, sId, cId? memId, stgId, cdtId?

I'm ok with mId, sId & cId.
stg & cdt seem weird

[MerlinEgalite]

another option is to indeed use idCalldata, idMemory & idStorage but use an alias when importing a specific function. Not sure I prefer this way though.

[Rubilmax]

another option is to indeed use idCalldata, idMemory & idStorage but use an alias when importing a specific function. Not sure I prefer this way though.

This is neat! So we can minimize changes in Blue (if we really want to lol). But is there a syntax for it in Solidity, for internal library functions?

[MerlinEgalite]

another option is to indeed use idCalldata, idMemory & idStorage but use an alias when importing a specific function. Not sure I prefer this way though.

This is neat! So we can minimize changes in Blue (if we really want to lol). But is there a syntax for it in Solidity, for internal library functions?

I'll try it!

EDIT: seems impossible indeed...

[MathisGD]

But calldata is actually cheaper so I don't think we should this tbh.

Did you try it ? I'm not so sure

If it's not, I'm in favor of using memory everywhere and don't apply this change which is very ugly.

[makcandrov]

Did you try it ? I'm not so sure

In our case, indeed, specifying calldata doesn't bring any optimization because the first thing we do in every function is to hash the data (to get the id). Hashing requires having the data in memory, thus copying it into memory if it was not (which is exactly what specifying calldata avoids doing).

[MerlinEgalite]

So should I go back to id and idStorage?

[Rubilmax]

So should I go back to id and idStorage?

I'm ok with id and sId as well as mId & sId as well as memId & stgId. But if you really want idStorage, I won't block the discussion because it's just that I find it ugly.

[MerlinEgalite]

Actually using memory saves gas except for the liquidation: