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

Implement extcodehash #77

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ermalkaleci
Copy link
Contributor

@ermalkaleci ermalkaleci commented Oct 11, 2024

  • Add manual tests for extcodehash because we can't hardcode the hash
  • Update specs instantiate result to include code hash

TODO:

  • Update pallet-revive to return hardcoded hash for EOA

@ermalkaleci ermalkaleci marked this pull request as ready for review October 11, 2024 08:59
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request!

crates/llvm-context/src/polkavm/context/mod.rs Outdated Show resolved Hide resolved
crates/llvm-context/src/polkavm/evm/ext_code.rs Outdated Show resolved Hide resolved
crates/integration/contracts/ExtCode.sol Outdated Show resolved Hide resolved

let extcodehash_pointer = context.build_alloca(context.word_type(), "extcodehash_pointer");

context.build_runtime_call(
Copy link
Member

@xermicus xermicus Oct 11, 2024

Choose a reason for hiding this comment

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

This does not fully resemble the EVM semantics: hash of the chosen account's code, the empty hash (0xc5d24601...) if the account has no code, or 0 if the account does not exist or has been destroyed. Here is the syscall for reference.

I think with the current implementation in pallet-revive, we are basically forced to emit some branches and make two syscalls for this. Which is not desired, everything the runtime can do for us easily it should do (it is faster and spares contract code size). So this needs a PR in the pallet first to let it distinguish between contract-accounts, no-contract accounts and unknown accounts and write back the correct value automatically. Then it can be properly implemented and tested.

Sorry in the corresponding issue #63 I should have wrote that out more clearly.

@xermicus
Copy link
Member

Closes #63. @ermalkaleci if you feel like takling this on the runtime side too, can I assign #63 to you?

@ermalkaleci
Copy link
Contributor Author

Absolutely, I would love to contribute. Hope this is good start :)

@ermalkaleci ermalkaleci marked this pull request as draft October 11, 2024 10:33
@xermicus
Copy link
Member

Nice, thank you! Yeah we can take it from here. There's nothing complex to it, it is a good first issue.

@ermalkaleci
Copy link
Contributor Author

@xermicus can you give this a review?

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.

2 participants