Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Jul 29 2020 01:16
  • Jul 22 2020 07:07
  • 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
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
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
JinGyeong Jeong
@joojis
I think there is a point of pain when a new contributor is trying to patch RPC because of the e2e tests.
I think we should change our practice to use sendRpcRequest function in our e2e tests so that no SDK publish is required.
Kwang Yul Seo
@kseo
@sgkim126 would you review this PR again?
CodeChain-io/foundry#12