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

Not implemented chain_id function. #44

Closed
smiasojed opened this issue Sep 12, 2024 · 11 comments · Fixed by #54
Closed

Not implemented chain_id function. #44

smiasojed opened this issue Sep 12, 2024 · 11 comments · Fixed by #54
Assignees

Comments

@smiasojed
Copy link
Collaborator

smiasojed commented Sep 12, 2024

The ERC20 workspace in Remix is failing to compile.

How to reproduce:

  1. Got to https://smiasojed.github.io/remix-project
  2. Select ERC20 workspace
  3. Compile

results:

Contract `contracts/MyToken.sol:MyToken` compiling error: thread 'main' panicked at /home/runner/work/revive/revive/crates/llvm-context/src/polkavm/evm/context.rs:46:5:
not yet implemented
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

eip: https://eips.ethereum.org/EIPS/eip-1344

@smiasojed smiasojed changed the title Not implemented chain_id function. Not implemented chain_id function. Sep 12, 2024
@xermicus
Copy link
Member

Can you rebase against latest main where it will always return a chain id of 0 instead of panicking? I snuck it in recently. We still have to agree on those.

@xermicus xermicus self-assigned this Sep 12, 2024
@smiasojed
Copy link
Collaborator Author

smiasojed commented Sep 18, 2024

With the new version I got another panic:
Contract contracts/MyToken.sol:MyToken compiling error: thread 'main' panicked at /Users/smiasojed/Development/revive/crates/llvm-context/src/polkavm/context/pointer.rs:65:9:\nassertion left != right failed: Stack pointers cannot be addressed\n left: Stack\n right: Stack\n

@xermicus
Copy link
Member

Can you provide me the contents of MyToken.sol? I don't see the ERC20 workspace you mentioned originally

@xermicus
Copy link
Member

Also any libraries, assuming it uses OZ

@xermicus
Copy link
Member

Nevermind. It is caused by the immutables, which are still a WIP.

@smiasojed
Copy link
Collaborator Author

smiasojed commented Sep 18, 2024

Can you provide me the contents of MyToken.sol? I don't see the ERC20 workspace you mentioned originally

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";

contract MyToken is ERC20, ERC20Permit {
    constructor() ERC20("MyToken", "MTK") ERC20Permit("MyToken") {}
}

on the remix main page in the section: Explore. Prototype. Create. Click ERC20

@athei
Copy link
Member

athei commented Sep 18, 2024

I guess chain_id needs pallet support to implement properly. We should check what other L2, Moonbeam etc. return there.

@xermicus
Copy link
Member

I planned to add it to the pallet as configuration. Eventually we'll have to settle on the ChainID for every chain we deploy, including testnets. So having it in the configuration seems reasonable to me.

@athei
Copy link
Member

athei commented Sep 18, 2024

One obvious idea would be to use the parachain id. But it is not unique across relay chains. So maybe something like keccak("revive_chain" ++ relay_chain_id ++ parachain_id). But no matter what we pick: Exposing it as Config` item is the way to go as it has to be configured by the runtime.

But I assume it doesn't really matter as long it is unique and people can lookup what is what.

@xermicus
Copy link
Member

Yeah, we might want to add them here too.

@athei
Copy link
Member

athei commented Sep 18, 2024

Yes I was wondering how if numbers are coordinated. So basically just pick a unique one and get a PR merged. No hashing required :)

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Sep 23, 2024
This PR adds the EVM chain ID to Config as well as a corresponding
runtime API so contracts can query it.

Related issue: paritytech/revive#44

---------

Signed-off-by: xermicus <[email protected]>
Co-authored-by: command-bot <>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Sep 23, 2024
This PR adds the EVM chain ID to Config as well as a corresponding
runtime API so contracts can query it.

Related issue: paritytech/revive#44

---------

Signed-off-by: xermicus <[email protected]>
Co-authored-by: command-bot <>
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 a pull request may close this issue.

3 participants