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

Common: Add isPrecompiled method #727

Closed
holgerd77 opened this issue Apr 24, 2020 · 6 comments
Closed

Common: Add isPrecompiled method #727

holgerd77 opened this issue Apr 24, 2020 · 6 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Apr 24, 2020

The isPrecompiled method is currently being removed from the ethereumjs-util library, since the library has no notion of a "hardfork" and the method is therefore pretty error-prone.

Instead isPrecompiled might be a better fit in the Common class and give out the respective answers depending on the HF set on the common instance.

Not sure if this is needed or if there is a strong demand, just drop this here for some eventual discussion and/or remembrance.

@jochem-brouwer
Copy link
Member

This should have been exposed in this PR. It is trivial, I'll add it.

@jochem-brouwer jochem-brouwer self-assigned this Jun 29, 2020
@jochem-brouwer
Copy link
Member

Actually it is not trivial where we should expose this function. Should we expose this on top of the VM? This only has things like runCall, runBlock, etc. Feels a bit weird to put this "utility" function there. It feels more natural to put it in the EVM, but the VM does not appear to have a pointer to the EVM? Where should we expose this function?

I think it is good to think about this, as we might get demand for some more of these utility-like functions. We need to know where to place those.

@holgerd77
Copy link
Member Author

Ah - attention! 😄 - this is an issue associated to the Common library, so "here" means "Common" in this case.

So this would be a utility function in Common to return the precompile status of an account based on some precompile info which would need to be added to the HF files.

Not sure if it's worth to be added right now tbh, would be nice to have but also some significant effort to integrate.

@jochem-brouwer
Copy link
Member

Oops - I didn't see that this was actually the Common package.

We have a slight problem here as currently the hardforks are hard-coded (hah!) in index.ts of precompiles. It would make more sense to let the precompiles call into Common: getPrecompile actually has access to Common now so this should not be a problem.

I currently don't know how we should do this in a clean way. We could maybe create a map in common where we loop over each fork, latest first, and either return a precompile (I guess this should be an enum so we can support different precompiles at the same address if this happens at some point (?)) or we return that no precompile exists at this address? And then build the trivial isPrecompiled on top of that.

@jochem-brouwer jochem-brouwer mentioned this issue Feb 22, 2021
4 tasks
@holgerd77 holgerd77 changed the title Add isPrecompiled method Common: Add isPrecompiled method Jan 13, 2022
@jochem-brouwer
Copy link
Member

I am not sure if this is possible in Common anymore, since we now have custom precompiles, which Common is not aware of?

@holgerd77
Copy link
Member Author

I will close this issue for now since it is open for 2+ years and was in discussion respectively not completely clear if wanted/needed from the beginning.

Fell free to re-open if the need (re-)arises.

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

No branches or pull requests

2 participants