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

feat: VVM injection, internal functions and variables #294

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

DanielSchiavini
Copy link
Collaborator

@DanielSchiavini DanielSchiavini commented Sep 2, 2024

What I did

  • Add support for inject_function, internal and _storage contract methods for any contract compiled via the VVM
  • The performance is probably not great, since many recompiles are currently used
  • However, this might be still useful for users that want to upgrade boa and keep 0.3 vyper code around

How I did it

  • The methods recompile a version of the contract with a new internal method
  • When the method is called, it will temporarily change the bytecode at the destination
  • This is of course not very performant, but may be improved later

Cute Animal Picture

image

@DanielSchiavini DanielSchiavini marked this pull request as ready for review September 3, 2024 09:03
@DanielSchiavini
Copy link
Collaborator Author

I'm waiting for vyperlang/vvm#21 and vyperlang/vvm#23 before finishing this PR

@DanielSchiavini DanielSchiavini changed the title VVM eval, internal functions and variables feat: VVM eval, internal functions and variables Sep 23, 2024
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
boa/interpret.py Outdated Show resolved Hide resolved
return None

result = vvm.compile_source(
self.source_code, vyper_version=self.vyper_version, output_format="metadata"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just get metadata to begin with (when generating VVMDeployer)?

also, note that metadata isn't available in all versions of vyper, and it is not guaranteed to be stable between releases. but this probably works for recent vyper versions (0.3.7-0.3.10)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth I would have no objection to restricting the scope of the VVM functionalities to 0.3.7+

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just get metadata to begin with (when generating VVMDeployer)?

Can we do that without calling the compiler twice? I implemented it here so it's only done when necessary (note the cached_property)

also, note that metadata isn't available in all versions of Vyper

We'll show the error from the compiler, so it should be clear when not supported

not guaranteed to be stable between releases

If you way to get info about private functions that's stable please let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just get metadata to begin with (when generating VVMDeployer)?

Can we do that without calling the compiler twice? I implemented it here so it's only done when necessary (note the cached_property)

ah, maybe we should have added the option for multiple output formats in vvm

Copy link
Collaborator Author

@DanielSchiavini DanielSchiavini Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, we should probably add "stabilize" metadata (which, i think we have mostly stopped making changes to it) and add to combined_json output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a good idea, however that won't work for old vyper versions

@DanielSchiavini DanielSchiavini changed the title feat: VVM eval, internal functions and variables feat: VVM injection, internal functions and variables Oct 18, 2024
self.contract = contract

def get(self, *args):
return self.__call__(*args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really work the same way as VyperContract.get()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please explain

Copy link
Member

@charles-cooper charles-cooper Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in vyper_contract.StorageVar.get(), it takes no arguments, and instead iterates over touched storage slots to reconstruct a dict. the way it's done here, it exposes a getter which the user needs to provide arguments to

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am mildly ok with not requiring the API to be the same, but in that case we should probably call it something else, like get_value_at()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK, I thought that's how that worked, I might have mixed it up when implementing zksync plugin.
I don't think we can get all the key values via the ABI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants