Skip to content
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

[LS] Add InternalEVM stdlib Contract #331

Closed
jribbink opened this issue Apr 2, 2024 · 3 comments · Fixed by #332
Closed

[LS] Add InternalEVM stdlib Contract #331

jribbink opened this issue Apr 2, 2024 · 3 comments · Fixed by #332
Assignees

Comments

@jribbink
Copy link
Contributor

jribbink commented Apr 2, 2024

Issue to be solved

Because the InternalEVM standard library value is not a part of the Cadence runtime, but rather, part of the FVM, it is not known to the language server. It is, however, valid to the FVM & developers wishing to get code-analysis while using the EVM/InternalEVM contract will be unable to.

This is a more general problem than just the InternalEVM contract, however, it seems to me that the InternalEVM contract is the only one that needs to be considered for the time being.

(related onflow/vscode-cadence#574)

Suggested Solution

Add an additional option to the server, WithStandardLibraryValues, or similar, that would accept additional values that may be declared as a part of the stdlib.

This could be used as a part of the LS flow integration, where the FVM standard library values are specified. This way the FVM-specific stdlib stays decoupled from the Cadence LS implementation itself.

@jribbink jribbink changed the title [LS] Add InternalEVM (& ability to add any additional FVM stdlib items) [LS] Add InternalEVM stdlib value Apr 2, 2024
@jribbink jribbink changed the title [LS] Add InternalEVM stdlib value [LS] Add InternalEVM stdlib Contract Apr 2, 2024
@jribbink jribbink moved this to 👀 In Review in 🌊 Flow 4D Apr 3, 2024
@turbolent
Copy link
Member

turbolent commented Apr 3, 2024

InternalEVM is an implementation detail, only used and accessible in the EVM contract.

While it is annoying that the EVM contract (https://github.com/onflow/flow-go/blob/master/fvm/evm/stdlib/contract.cdc) cannot be successfully checked/developed with the LS, this should only really be a problem for contributors to flow-go/FVM, no?

@jribbink
Copy link
Contributor Author

jribbink commented Apr 3, 2024

InternalEVM is an implementation detail, only used and accessible in the EVM contract.

While it is annoying that the EVM contract (https://github.com/onflow/flow-go/blob/master/fvm/evm/stdlib/contract.cdc) cannot be successfully checked/developed with the LS, this should only really be a problem for contributors to flow-go/FVM, no?

Unfortunately, the problem actually surfaces for developers importing the EVM contract. What happens is that, because the EVM contract produces checker errors (by relying on InternalEVM), the import cannot be resolved, so it is inaccessible within the user's code.

Screen Shot 2024-04-03 at 2 17 48 PM

I guess the other workaround here is trying to ignore errors when checking EVM contract, although I worry that it may be a slippery slope trying to write custom import rules for a particular imported contract.

@turbolent
Copy link
Member

Unfortunately, the problem actually surfaces for developers importing the EVM contract. What happens is that, because the EVM contract produces checker errors (by relying on InternalEVM), the import cannot be resolved, so it is inaccessible within the user's code.

Ah, that makes sense! I hadn't considered that even for code importing EVM this is an issue. 👍

@jribbink jribbink self-assigned this Apr 25, 2024
@gregsantos gregsantos moved this from 👀 In Review to 🏗 In Progress in 🌊 Flow 4D Apr 25, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in 🌊 Flow 4D Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants