paritytech / revive

Solidity compiler for PolkaVM
Apache License 2.0
38 stars 5 forks source link

Not implemented `chain_id` function. #44

Closed smiasojed closed 1 month ago

smiasojed commented 2 months ago

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

xermicus commented 2 months ago

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.

smiasojed commented 1 month ago

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 commented 1 month ago

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

xermicus commented 1 month ago

Also any libraries, assuming it uses OZ

xermicus commented 1 month ago

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

smiasojed commented 1 month ago

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 commented 1 month ago

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

xermicus commented 1 month ago

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 commented 1 month ago

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 asConfig` 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 commented 1 month ago

Yeah, we might want to add them here too.

athei commented 1 month ago

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