Where communities thrive

Join over 1.5M+ people
Join over 100K+ communities
Free without limits
Create your own community

Explore more communities

CodeChain-io/codechain

Programmable multi-asset chain

People

Repo info

Activity

Jul 29 2020 01:16

Travis CodeChain-io/codechain-rpc-js (master) passed (38)
Jul 22 2020 07:07

Travis CodeChain-io/codechain-rpc-js#14 passed (37)
Jul 22 2020 07:07

Travis kseo/codechain-rpc-js (license) passed (1)
Mar 20 2020 06:28

MSNTCS opened #1962
Mar 20 2020 06:27

MSNTCS commented #1961
Mar 20 2020 06:25

MSNTCS closed #1961
Mar 19 2020 08:02

MSNTCS review_requested #1961
Mar 19 2020 08:02

MSNTCS opened #1961
Mar 03 2020 02:37

ScarletBlue edited #1948
Mar 03 2020 02:37

ScarletBlue edited #1931
Mar 02 2020 00:41

ScarletBlue edited #1915
Mar 02 2020 00:41

ScarletBlue edited #1915
Feb 27 2020 00:09

sgkim126 closed #1959
Feb 27 2020 00:09

sgkim126 on master

Bug fix in on_request_commit_me… (compare)
Feb 25 2020 09:41

majecty labeled #1960
Feb 25 2020 09:41

majecty opened #1960
Feb 17 2020 02:10

majecty commented #1959
Feb 14 2020 23:13

sgkim126 review_requested #1959
Feb 14 2020 23:13

sgkim126 review_requested #1959
Feb 14 2020 23:13

sgkim126 opened #1959

Joonmo Yang

@Remagpie

I like this Commit in the PR. Before the commit, code is divided according to whether the validator set is dynamic or static. After the commit, the code is split by what it does first and split again by the dynamic or static validator set. Thanks the change, it is easy to track what's different between the dynamic validator set and the static validator set.

:smile:

Park Juhyung

@majecty

There is a hidden assumption that move_to_step should be called after jump_to_height function is called. I'll add comments about the assumption in the jump_to_height function.

JinGyeong Jeong

@joojis

I will review CodeChain-io/codechain#1920 now

Joonmo Yang

@Remagpie

Is there any reason why we're using chai-as-promised for the e2e tests?
I thought async/await and try/catch would be enough.

JinGyeong Jeong

@joojis

@Remagpie I guess there’s no special reason.

Seulgi Kim

@sgkim126

@Remagpie I think it's a matter of taste. chai-as-promised shows the intention clearly by the name rejected/rejectedWith/fulfilled, but it also needs boilerplate code to import.

Seulgi Kim

@sgkim126

@/all Have we used the null consensus engine? Why not removing it?

Park Juhyung

@majecty

I've never seen code that using the null consensus engine. I agree with you. Let's remove it.

Park Juhyung

@majecty

I'm reviewing CodeChain-io/foundry#1. I'm happy to see this PR.

Joonmo Yang

@Remagpie

I think it can be useful for unit testings. Is solo consensus enough for the tests?

Seulgi Kim

@sgkim126

@Remagpie I'll check it. I think there is little difference between solo and null engine.

@kseo CodeChain-io/codechain#1930 please review it.

Kwang Yul Seo

@kseo

@sgkim126 merged

Kwang Yul Seo

@kseo

@sgkim126 do we need to merge CodeChain-io/codechain#1930 into foundrytoo?

Seulgi Kim

@sgkim126

@kseo Yes, we should. I'll do it soon.

Park Juhyung

@majecty

I'll remove Asset related code in Foundry. I'll modify these modules: State, RPC, and VM. I'll remove related e2e tests.

@sgkim126 Please tell me if you think there will be a significant merge conflict with your commits.

Kwang Yul Seo

@kseo

@sgkim126 are you working on CodeChain-io/foundry#1

Park Juhyung

@majecty

I've started removing Assets in Foundry.

Seulgi Kim

@sgkim126

@kseo Yes, I'm still working on. I'll remove the WIP prefix when finished.

@majecty The parts you touched are not related to my parts. I think only Cargo.lock will conflict.

Seulgi Kim

@sgkim126

@majecty I removed some RPCs also, but it will not be a problem.

Park Juhyung

@majecty

OK.

Seulgi Kim

@sgkim126

@Remagpie I created CodeChain-io/foundry#8 and CodeChain-io/foundry#9

Joonmo Yang

@Remagpie

:+1:

Park Juhyung

@majecty

@sgkim126 What do you think about running cargo fix --edition-idioms in the foundry project?
We are using extern crate in many cases, but from the 2018 edition, we don't need it in most cases.
https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html#no-more-extern-crate

Park Juhyung

@majecty

@sgkim126 I'll remove shard related code from the Foundry while removing Assets.

Seulgi Kim

@sgkim126

@majecty I totally agree about cargo fix --edition-idioms. Please do it.

Kwang Yul Seo

@kseo

@sgkim126 it’s already done.CodeChain-io/foundry#10

Park Juhyung

@majecty

I removed all Asset and Shared related code in Foundry. I'm fixing clippy errors.

Park Juhyung

@majecty

I'm removing Asset related e2e tests.

Park Juhyung

@majecty

@sgkim126 What do you think about adding a dummy transaction that uses shards? If we remove all asset-related transactions, there will be no transactions that use shard states.

I'm considering not to remove MintAsset instead of a new dummy transaction.

Park Juhyung

@majecty

I'll add ShardStore transaction in Foundry. It's hard to keep shard structure after removing all shard transactions.

The "ShardStore" transaction saves arbitrary data in shards.

Park Juhyung

@majecty

@Remagpie FYI, a snapshot in Foundry must contain shard data.

Joonmo Yang

@Remagpie

Thanks. I almost forgot that. I added some commits to the PRs I sent.

Seulgi Kim

@sgkim126

@majecty I think we'd better implement shard module for testing including a dummy transaction.

Park Juhyung

@majecty

OK. I'm adding a simple transaction that uses shards. I'll request your Review today. Then I'll remove other shard transactions.

Park Juhyung

@majecty

Our check_shard_level_state macro is an excellent point to study Rust macro. I understood how to use the macro by reading it.

https://github.com/CodeChain-io/foundry/blob/master/state/src/impls/test_helper.rs#L558

Joonmo Yang

@Remagpie

Are we maintaining simple_poa consensus in foundry? I can't remember the usage of it.

Seulgi Kim

@sgkim126

The purpose was a testing network consensus without tendermint logic, but we don't use it for now.

Seulgi Kim

@sgkim126

I'll do CodeChain-io/foundry#21, CodeChain-io/foundry#14 would be better doing after snapshot refactoring.

Seulgi Kim

@sgkim126

Hi @majecty and @kseo, While I'm reviewing and writing commits, I found that the current coding style about importing order is easy to make a mistake. https://github.com/CodeChain-io/codechain/wiki/Coding-Style Currently, we split imports into 3 groups that are systems, other packages, and self modules, and sorting them in a group, but there are no formatting tools that group imports according to prefixes.

So we split the groups with a blank line, but we should take care of whether they are grouped as intended; it's easy to make a mistake.

I suggest to remove groups and make all imports ordered alphabetically. AFAIK, rust compilation doesn't affect by the importing order; therefore, it will not make a problem.

Kwang Yul Seo

@kseo

@sgkim126 sounds good to me. @majecty what’s your opinion?

Park Juhyung

@majecty

I'm happy to hear the suggestion. Let's do it!

Kwang Yul Seo

@kseo

@sgkim126 please file an issue first

Seulgi Kim

@sgkim126

I made two issues CodeChain-io/codechain#1940 and CodeChain-io/foundry#41