darwinia-network / darwinia-common

Darwinia Runtime Pallet Library and Pangolin/Pangoro Testnet
https://rust-docs.darwinia.network/darwinia-common
GNU General Public License v3.0
30 stars 9 forks source link

[DVM] Incorrect data display #339

Closed boundless-forest closed 4 years ago

boundless-forest commented 4 years ago

Desc

Currently, based on the implementation of the common node, the ethereum web3 SDK can be used for transfer. However, the balance displayed after the transfer is incorrect, which may be due to accuracy problems or other reasons.

The process

Firstly, Try to get balance of 0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b via web3js.

const Web3 = require('web3');

// Variables definition
const addressFrom = '0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b';
const web3 = new Web3('http://localhost:9933');

// Balance call
const balances = async () => {
   const balanceFrom = web3.utils.fromWei(
      await web3.eth.getBalance(addressFrom),
      'ether'
   );

   console.log(`The balance of ${addressFrom} is: ${balanceFrom} ETH.`);
};

balances();

The output result is 123356.123

The balance of 0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b is: 123356.123 ETH.

However, When i try to get balance of substrate address which 0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b refer to via apps or debug log.

The log show like this:

Oct 22 16:17:53.389  INFO bear--- here, address 0x6be02d1d3665660d22ff9624b7be0551ee1ac91b    
Oct 22 16:17:53.390  INFO bear --- the balance number 123456123000000000000000 

The chain balance is 123456123000000000000000

In another way, the address 0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b's chain balance is 123456123000000000000000, and the result displayed via web3, metamask is 123356.123. which may be due to accuracy problems or other reasons.

boundless-forest commented 4 years ago

This issue can be fixed via change ether unit usage.

const Web3 = require('web3');

// Variables definition
const privKey =
   '99B3C12287537E38C90A9219D4CB074A89A16E9CDB20BF85728EBD97C343E342';
const addressFrom = '0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b';
const addressTo = '0xB90168C8CBcd351D069ffFdA7B71cd846924d551';
const web3 = new Web3('http://localhost:9933');

// Create transaction
const deploy = async () => {
    console.log(
       `Attempting to send transaction from ${addressFrom} to ${addressTo}`
    );

    const createTransaction = await web3.eth.accounts.signTransaction(
       {
          from: addressFrom,
          to: addressTo,
         //  value: web3.utils.toWei('100', 'ether'),
          value: 10000000000,
          gas: '4294967295',
       },
       privKey
    );

    const createReceipt = await web3.eth.sendSignedTransaction(
        createTransaction.rawTransaction
    );

    console.log(
        `Transaction successful with hash: ${createReceipt.transactionHash}`
    );
}

deploy();
const Web3 = require('web3');

// Variables definition
const addressFrom = '0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b';
const addressTo = '0xB90168C8CBcd351D069ffFdA7B71cd846924d551';
const web3 = new Web3('http://localhost:9933');

// Balance call
const balances = async () => {
   const balanceFrom = web3.utils.fromWei(
      await web3.eth.getBalance(addressFrom),
      // 'ether'
      'wei'
   );
   const balanceTo = await web3.utils.fromWei(
      await web3.eth.getBalance(addressTo),
      // 'ether'
      'wei'
   );

   console.log(`The balance of ${addressFrom} is: ${balanceFrom} ETH.`);
   console.log(`The balance of ${addressTo} is: ${balanceTo} ETH.`);
};

balances();
hackfisher commented 4 years ago

Did you try metamask? can metamask customize native currency decimal?that's an more important issue for the dvm solution.

hackfisher commented 4 years ago

RING's minimum unit is nano, 1nano = 10^9 wei. Maybe we can take a look at the web3 get_balance rpc implement, convert from nano(storage unit) to wei there?

In solidity, developers should also be careful what's the unit in use(currently nano, just like sun in Tron), should be caution when using wei(not exist, 10^-9 nano,parsed as 1?) or ether

If metamask does not support change default decimal, to use metamask, we need

1 RING = 1 ether = 10^9 nano = 10^18 wei

boundless-forest commented 4 years ago

Did you try metamask? can metamask customize native currency decimal?that's an more important issue for the dvm solution.

Yep, but not found the setting about currency decimal. I have talked about this issue with @WoeOm who is more familiar with metamask. He might have some trick skills to deal with this problem.

boundless-forest commented 4 years ago

RING's minimum unit is nano, 1nano = 10^9 wei. Maybe we can take a look at the web3 get_balance rpc implement, convert from nano(storage unit) to wei there?

Indeed. We could do this change in web3, but metaMask won't use our folked web3. That sounds like not a good way.

hackfisher commented 4 years ago

RING's minimum unit is nano, 1nano = 10^9 wei. Maybe we can take a look at the web3 get_balance rpc implement, convert from nano(storage unit) to wei there?

Indeed. We could do this change in web3, but metaMask won't use our folked web3. That sounds like not a good way.

I mean the balance expose part in nodes, not web3.js.

https://github.com/paritytech/substrate/blob/019019fff21a2ba7128eb3c23d8f65e9bb03caee/frame/evm/src/lib.rs#L475

WoeOm commented 4 years ago

In dvm, transfer 1 wei to a contract, and then check the balance of the contract, will it be 0 or 1 wei?

hackfisher commented 4 years ago

In dvm, transfer 1 wei to a contract, and then check the balance of the contract, will it be 0 or 1 wei?

I guess 0, unless we change the storage value to U128 and decimal to 18?

hackfisher commented 4 years ago

Some practices from Tron:

https://developers.tron.network/docs/vm-vs-evm

https://developers.tron.network/docs/the-special-constants-differ-from-ethereum

https://developers.tron.network/reference#fromsun

The difference between DVM and Tron VM is that we want users be able to use metamask seamlessly while Tron have its own wallets (Tron Link).

To achieve this, we may need to do some hack to the node rpc services(get_balance, transfer) to adapt the currency decimals between them, (Metamask and web3.js)using unit wei instead.

As a side effect, all value < 10^9 wei might be ignored (Warning in developer docs).

The decimal of RING currency inside DVM still keep 9, unit nano (just like sun in Tron VM)

Contract and ERC20 Tokens will not be affected, as DVM support U128, they can use any decimal they prefer.

boundless-forest commented 4 years ago

I think this is a feasible solution for this issue.

hackfisher commented 4 years ago

It looks like Metamask and web3 rpc spec does not support customize primary currency decimal in custom network RPC.

https://metamask.zendesk.com/hc/en-us/articles/360043227612-How-to-add-a-custom-Network-RPC-and-or-Block-Explorer

https://web3js.readthedocs.io/en/v1.3.0/web3-eth.html#id49 (web3 rpc docs also said the unit should be wei)

Didn't found any rpc method returns primary currency metadata, guess can not change decimal to other number other than 18 in web3 rpc spec.

boundless-forest commented 4 years ago

Yes, It seems impossible to fix metamask currency display problems by just custom config.

I had workeded on this change temporarily for poc, the web3 query balance result the same as apps shows.

https://github.com/darwinia-network/darwinia-common/blob/integrate-frontier-substrate-2.0.0/frame/dvm/rpc/src/eth.rs#L315-L326

fn balance(&self, address: H160, number: Option<BlockNumber>) -> Result<U256> {
        if let Ok(Some(id)) = self.native_block_id(number) {
            let balance = self.client
                .runtime_api()
                .account_basic(&id, address)
                .map_err(|err| internal_err(format!("fetch runtime chain id failed: {:?}", err)))?
                .balance
                .checked_mul(U256::from(1000000000)).unwrap()
                .into();
            debug::info!("bear --- the address {:?}, balance is {:?}", address, balance);

            // return Ok(self
            //  .client
            //  .runtime_api()
            //  .account_basic(&id, address)
            //  .map_err(|err| internal_err(format!("fetch runtime chain id failed: {:?}", err)))?
            //  .balance
            //  .into());
            return Ok(balance);
        }
        Ok(U256::zero())

Besides balance display, i am trying to figure out how to make gas_used adapt to this solution.

hackfisher commented 4 years ago

I'm not satisfied with the hack part of change transfer value spec, this may cause some unexpected behavior in smart contract, and confusion about how they see value, 10^18, or 10^9?

I'm thinking of another direction:

  1. The RING's value decimal in DVM is 10^18, just like ether. This will make web3 rpc & Matamask compatibility support more easier.

  2. The current balance module and storage are using 10^9, and can not store U256. so we will need to do a conversion when updating primary currency's balance storage(Aka. mutate_account_basic)

Thanks to substrate evm, there is already such public function in evm pallet:

https://github.com/paritytech/substrate/blob/7142b928ba8a66d4b7a9c7f4a92cdc04b5983dc7/frame/evm/src/lib.rs#L428

It is simply doing diff.low_u128() conversion currently, we may need change it to diff.div(10^9), the trailing 0 in the U256 value will be cut off when modifying account balance in DVM. That is updating diff value which < 10^9 in smart contract will not take any effect to the account balance, just like +=0 or do something like += x / 10^9 * 10^9 in Ethereum EVM.

This part of get balance need to be modified too, in here: https://github.com/paritytech/substrate/blob/master/frame/evm/src/lib.rs#L475

Changing

Account {
            nonce: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(nonce)),
            balance: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(balance)),
        }

to

Account {
            nonce: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(nonce)),
            balance: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(balance) * 10^ 9),
        }

We must call attention to smart contract developer about this knowledge in case of confusion.

We can keep an U256 dust value in DVM to track the lost balance part cause by the cut off, which can be good to consistency:

Sum(dust_u256_value + all_balances_u256_value) = Sum(DVM_adddresses_storage_u128_value) * 10^9