rooch-network / rooch

VApp Container with Move Language
https://rooch.network
Apache License 2.0
128 stars 54 forks source link

[style] code style for dealing with `E` variable in Move. #663

Closed feliciss closed 8 months ago

feliciss commented 9 months ago

Several implementations showed mishandling of constants. Such as:

/// there defines scheme for each blockchain
const ETHEREUM_SCHEME: u64 = 3;

And to tackle with it, some implementations suggested using a prefix V, for example:

/// constant codes
const V_ECDSA_K1_RECOVERABLE_TO_ETHEREUM_SCHEME_LENGTH: u64 = 1;

I suggest to unify the coding style to avoid the E starting word. It should be started with VALID instead of V which indicates a variable:

const VALID_ETHEREUM_ADDR_LENGTH: u64 = 20;

Because the const in move with every capital words already indicates it's a variable.

Update:

I think the upper snake case is okay for beginning with E? @jolestar

Reference:

jolestar commented 9 months ago

VALID as a prefix looks a bit strange, like exist another INVALID prefix const. Or we put CONST as a prefix, const CONST_ ETHEREUM_SCHEME. Throught this is a redundant prefix, but it has no side effect.

jolestar commented 9 months ago

@baichuan3 @stevenlaw123 @wow-sven

baichuan3 commented 9 months ago

VALID as a prefix looks a bit strange, like exist another INVALID prefix const. Or we put CONST as a prefix, const CONST_ ETHEREUM_SCHEME. Throught this is a redundant prefix, but it has no side effect.

it's ok

stevenlaw123 commented 9 months ago

Starting with E_ as error codes and using other variable names for non-error variables should be able to distinguish between them.

feliciss commented 9 months ago

Starting with E_ as error codes and using other variable names for non-error variables should be able to distinguish between them.

I second this.

jolestar commented 9 months ago

Starting with E_ as error codes and using other variable names for non-error variables should be able to distinguish between them.

EIndexOutOfBounds => E_IndexOutOfBounds? is a little strange. Or EIndexOutOfBounds => E_INDEX_OUT_OF_BOUNDS?

feliciss commented 9 months ago

Starting with E_ as error codes and using other variable names for non-error variables should be able to distinguish between them.

EIndexOutOfBounds => E_IndexOutOfBounds? is a little strange. Or EIndexOutOfBounds => E_INDEX_OUT_OF_BOUNDS?

The first one, as stated here:

Constant names: should be upper camel case and begin with an E if they represent error codes (e.g., EIndexOutOfBounds) and upper snake case if they represent a non-error value (e.g., MIN_STAKE).

https://move-language.github.io/move/coding-conventions.html.

Errors should be upper camel case.

In this version, it's upper snake camel case.

Or I would suggest EIndexOutOfBounds => ErrorIndexOutOfBounds. That way, it wouldn't change the convention.

jolestar commented 9 months ago

We can discuss this at the dev meeting. But the error code in move stdlib is not following the coding conventions. We need to handle this for different frameworks.

https://github.com/rooch-network/rooch/blob/cdc5d3d9a3bcae115b67166168effc541e282503/moveos/moveos-stdlib/move-stdlib/sources/vector.move#L13-L15

jolestar commented 8 months ago
  1. ErrorIndexOutOfBounds & EIndexOutOfBounds, Different frameworks use a different prefix.
  2. CONST_ETHEREUM_SCHEME or C_ETHEREUM_SCHEME.
  3. E_INDEX_OUT_OF_BOUNDS.
feliciss commented 8 months ago
  • ErrorIndexOutOfBounds & EIndexOutOfBounds, Different frameworks use a different prefix.
  • CONST_ETHEREUM_SCHEME or C_ETHEREUM_SCHEME.
  • E_INDEX_OUT_OF_BOUNDS.

1 looks good?

jolestar commented 8 months ago

Yes, we need to configure the error code prefix for each framework.

MoveStd -> E MoveosStd -> Error RoochFramework -> Error

feliciss commented 8 months ago

Yes, we need to configure the error code prefix for each framework.

MoveStd -> E MoveosStd -> Error RoochFramework -> Error

Okay. I'll investigate.

feliciss commented 8 months ago

Yes, we need to configure the error code prefix for each framework.

MoveStd -> E MoveosStd -> Error RoochFramework -> Error

I referenced examples folder to enforce the compliance as same as MoveosStd and RoochFramework.