NethermindEth / warp

Warp - Bringing Solidity to Starknet at warp speed. Warp is a Solidity to Cairo Compiler, this allows teams to write/migrate Solidity to Cairo for easy onboarding into the StarkNet ecosystem.
https://nethermind.io/warp/
Apache License 2.0
754 stars 70 forks source link

remove the use of implicits that are not warp-memory #1022

Closed AlejandroLabourdette closed 1 year ago

AlejandroLabourdette commented 1 year ago

I'd suggest you to add warp_memory implicit rather than completely getting rid of it

#[implicit(warp_memory)]
fn f(...) {
  ...
  warp_memory.read_u128(index);
  ...
}

It would be difficult later to track places where warp_memory needs to be added if we remove it from all of the places now.

I agree 100% with you, actually that's was what I think, that's why on CairoFunctionDefinition implementation I appended a comment of how we plan to use implicit and sort of what's happening with them. https://github.com/NethermindEth/warp/blob/5cc3efe06dfff437b11b68929a0385b557bbbfba/src/ast/cairoNodes/cairoFunctionDefinition.ts#L18

Right now we still have a set of Implicits foreach function definition. Even considering that the only available implicit is warplib_memory the logic we had to infer the implicits of the current function worth it.

Nevertheless I do remove them from cairoUtilFuncGen scripts that were not updated to Cairo1 syntax, mainly because we had them hardcode in there (was just a string that we insert in the raw string definition of those functions). They didn't have any role at ast level and we do have to review each of of those script to make them Cairo1 compatible. This is an example: https://github.com/NethermindEth/warp/blob/db1b5987c7fe19c0f7b26dc80239f819be7f644e/src/cairoUtilFuncGen/memory/implicitConversion.ts#L51 https://github.com/NethermindEth/warp/blob/db1b5987c7fe19c0f7b26dc80239f819be7f644e/src/cairoUtilFuncGen/memory/implicitConversion.ts#L210

piwonskp commented 1 year ago

I'd suggest you to add warp_memory implicit rather than completely getting rid of it

#[implicit(warp_memory)]
fn f(...) {
  ...
  warp_memory.read_u128(index);
  ...
}

It would be difficult later to track places where warp_memory needs to be added if we remove it from all of the places now.

I agree. Let's just put #[implicit(warp_memory)] in front of those functions, but without updating the rest of their implementation and syntax

rjnrohit commented 1 year ago

Oops ! conflicts has come up since I have merged the uintN pr, could you resolve it?

AlejandroLabourdette commented 1 year ago

As part of this PR you should update cairoParsing.ts and make it parse the custom implicit attribute in order to detect when generated functions requires the warp_memory implicit

Actually that's already done, we just have the remains of the old parseImplicit, but using getRawCairoFunctionInfo is already known the implicit used (if used any). I'm gonna remove that old function right now

piwonskp commented 1 year ago

Does parseMultipleRawCairoFunctions work properly?

piwonskp commented 1 year ago

There is also getImplicitsAt in ast.ts. Is it needed?