Call the built-in Metadata contract causes an error.

[yangby-cryptape]

Current Behavior

Summary

Call the built-in Metadata contract causes an error:

CKB-VM error execution reverted

How to reproduce?

• Create a interface file for Metadata contract.

  • Base on the built-in Metadata contract SOL file.

  • Base on the internal Metadata contract ABI file.

• Call the method getMetadata(uint64 epoch) from another contract.

  function getMetadata(uint64 epoch) public view returns (IMetadataManager.Metadata memory){ 
    address addr = 0xFFfffFFfFFfffFfFffFFfFfFfFffFfffFFFFFf01;
    return IMetadataManager(addr).getMetadata(epoch);
  }

  References:

  • Method signature in the built-in Metadata contract SOL file.

    axon/builtin-contract/metadata/contracts/metadata.sol

    Line 139 in 71f5bad

    function getMetadata(uint64 epoch) public view returns (Metadata memory) {

  • Method signature in the internal Metadata contract ABI file.

    axon/core/executor/src/system_contract/metadata/abi/metadata_abi.json

    Lines 141 to 148 in 71f5bad

    	"inputs": [
    	{
    	"internalType": "uint64",
    	"name": "epoch",
    	"type": "uint64"
    	}
    	],
    	"name": "getMetadata",

There are several issues:

• Why the error message contains "CKB-VM"?

  I didn't call any contract which uses CKB-VM.

• How to call Metadata contract in Solidity scripts with the provided method?

  The provided method is:

  axon/core/executor/src/precompiles/metadata.rs

  Lines 11 to 17 in 71f5bad

  	/// The input argument must includes 9 bytes schema:
  	/// input[0]: an u8 on behalf of the call type. 0u8 means get metadata by block
  	/// number and 1u8 means get metadata by epoch number.
  	/// input[1..9]: 8 bytes of a **little endian** u64 value. If call type is 0u8,
  	/// it means a block number. If call type is 1u8, it means a epoch number.
  	#[derive(Default, Clone)]
  	pub struct Metadata;

  p.s. Axon should NOT force users to write assembly.

• Should there be some tests before new changes on the built-in Metadata contract? (ref: refactor!: insert metadata directly and remove genesis transactions #1454)

  Did I miss some things?
  I didn't find any tests under the test directory for the built-in Metadata contract.

  I'm neither an expert on JavaScript nor an expert on Solidity.
  Also, I haven't understood the storage model of Axon completely, yet.

  I hope someone can add such tests (or examples) to increase confidence for the changes of the built-in Metadata contract.

• Provide an interface SOL file for the built-in Metadata contract, and use unit tests to make sure it will be updated when the contract is changed.

  Axon should NOT let users to write the interface manually.

  It's better than provide a contract SOL file without code for implementation. (ref: refactor: change metadata system contract and add annotation #1449)

Expected Behavior

.

[yangby-cryptape]

In the issue body, I told one of my opinion:

Provide an interface SOL file for the built-in Metadata contract, and use unit tests to make sure it will be updated when the contract is changed.

Now I withdraw my previous opinion.

We could NOT provide any interface of the Metadata contract.

One of those reasons is: If the interface is valid, then all Metadata items in mapping(uint64 => Metadata) metadata_set; should have the same structure at any block.

axon/builtin-contract/metadata/contracts/metadata.sol

Lines 57 to 58 in 6fe5777

	// to store all metadata with epoch as key
	mapping(uint64 => Metadata) metadata_set;

That means this mapping should be refreshed after any hardfork and fill all new-added fields to all previous items.

[yangby-cryptape]

The all data stored for this contract are incompatible with EVM state storage.

Here I just give one example:

axon/core/executor/src/system_contract/metadata/store.rs

Lines 93 to 96 in 6fe5777

	self.trie.insert(
	metadata.epoch.to_be_bytes().to_vec(),
	metadata.encode()?.to_vec(),
	)?;

The key for the following mapping should be keccak256(concat(epoch_bytes, mapping_slot_in_bytes)).

axon/builtin-contract/metadata/contracts/metadata.sol

Lines 57 to 58 in 6fe5777

	// to store all metadata with epoch as key
	mapping(uint64 => Metadata) metadata_set;

The slot here should be 0 or 1; I'm not sure now, since the type of 1st internal value in struct definition is constant.

axon/builtin-contract/metadata/contracts/metadata.sol

Line 8 in 6fe5777

uint64 constant U64_MAX = 2 ** 64 - 1;

I guess a constant may not cost a slot, but I haven't find any docs about it yet.

Summary

This also is a reason for "why we could NOT provide any interface of the Metadata contract".

• If we want to create an EVM-compatible contract, obviously the Metadata contract is NOT.

• If the Metadata contract can be EVM-incompatible, then a design document is required.

  To prove all data stored are in control (as design), not just a random implementation picked by the PR authors.

References

• How are mappings stored in smart contract storage?

[KaoImin]

The system contracts do not allow other sol contracts to be called directly, because they are all write functions. If you want to read the data contains by system contracts, Axon provides some precompile to do this.

[yangby-cryptape]

• So, why you put a SOL file in directory without any documents?

  And in a latest PR, you will update that SOL file.

  As a user, you want to me how to understand that?

• I didn't read data directly, I called the function which provided by axon developers in that SOL file.

  Was axon developers provided that method in that SOL file?

  Can't users use that method?

  So, any usages of that SOL file, except confuse users?

  • That method in the current revision.

    axon/builtin-contract/metadata/contracts/metadata.sol

    Lines 138 to 139 in 6fe5777

    	// get metadata from `metadata_set` by epoch
    	function getMetadata(uint64 epoch) public view returns (Metadata memory) {

  • That method in the next revision, which is in refactor: change metadata system contract and add annotation #1449 now.

    axon/builtin-contract/metadata/contracts/metadata.sol

    Line 71 in 7e8eb16

    function getMetadata(uint64 epoch) public view returns (Metadata memory) {}

[Flouse]

After #1449

The latest interfaces of Metadata contract is

axon/builtin-contract/system-contract/contracts/metadata/Metadata.sol

Lines 54 to 67 in 4c43ed4

	// **Notice**
	// This file only defines the interface of metadata contract. The real
	// implementation is in `core/executor/src/system_contract/metadata`.
	interface MetadataManager {
	function appendMetadata(MetadataType.Metadata memory metadata) external;
	function updateConsensusConfig(
	MetadataType.ConsensusConfig memory config
	) external;
	function setCkbRelatedInfo(
	MetadataType.CkbRelatedInfo memory info
	) external;
	}

• The getMetadata function was removed, instead by axon_getMetadataByNumber RPC,
  because the returns (IMetadataManager.Metadata) may be changed if a hardfork happened.

[Flouse]

• Why the error message contains "CKB-VM"?
  I didn't call any contract which uses CKB-VM.

fixed by https://github.com/axonweb3/axon/pull/1484/files#diff-7dedadc749f34a439efcf21029dbc46d5551c34bafd11e30faf0f6d3ee688650R57-R137