CortexTheseus icon indicating copy to clipboard operation
CortexTheseus copied to clipboard

Current code review

Open ucwong opened this issue 5 years ago • 11 comments

Review code by package, good to point out some questions

btw, https://github.com/CortexFoundation/torrentfs

ucwong avatar Jul 04 '20 05:07 ucwong

How about combining the different versions from Ethereum like Homestead, Byzantium, Istanbul and Berlin into our own version?

DhunterAO avatar Jul 04 '20 07:07 DhunterAO

How about combining the different versions from Ethereum like Homestead, Byzantium, Istanbul and Berlin into our own version?

combine them as a single one ?

ucwong avatar Jul 04 '20 07:07 ucwong

The Quota and QuotaUsed in Header are type big.Int now, why not use type uint64 as the GasLimit and GasUsed? Which can save space and time I think.

DhunterAO avatar Jul 04 '20 08:07 DhunterAO

How about combining the different versions from Ethereum like Homestead, Byzantium, Istanbul and Berlin into our own version?

combine them as a single one ?

Yeah, maybe give a name like Flush or something else. I think there is no blocks following the old rules in our chain, right?

DhunterAO avatar Jul 04 '20 08:07 DhunterAO

How about combining the different versions from Ethereum like Homestead, Byzantium, Istanbul and Berlin into our own version?

combine them as a single one ?

Yeah, maybe give a name like Flush or something else. I think there is no blocks following the old rules in our chain, right?

The Quota and QuotaUsed in Header are type big.Int now, why not use type uint64 as the GasLimit and GasUsed? Which can save space and time I think.

Yes, Ethereum is also doing something like this, use uint64 instead of big.int somewhere. It can improve some performance. You can take a look at it

ucwong avatar Jul 04 '20 08:07 ucwong

How about combining the different versions from Ethereum like Homestead, Byzantium, Istanbul and Berlin into our own version?

combine them as a single one ?

Yeah, maybe give a name like Flush or something else. I think there is no blocks following the old rules in our chain, right?

It's a good idea, we can think about the future version names, but for the passed version, in my opinion, I prefer to leave them as a history between us and Ethereum :)

ucwong avatar Jul 04 '20 08:07 ucwong

The Quota and QuotaUsed in Header are type big.Int now, why not use type uint64 as the GasLimit and GasUsed? Which can save space and time I think.

Yes, Ethereum is also doing something like this, use uint64 instead of big.int somewhere. It can improve some performance. You can take a look at it

Ok, I'll create a new PR for this.

DhunterAO avatar Jul 04 '20 08:07 DhunterAO

https://github.com/CortexFoundation/CortexTheseus/pull/654 Switch quota to uint64

ucwong avatar Jul 04 '20 10:07 ucwong

It seems that we could add gofmt function into Actions to prevent unformatted code. https://github.com/sjkaliski/go-github-actions

DhunterAO avatar Jul 23 '20 11:07 DhunterAO

It seems that we could add gofmt function into Actions to prevent unformatted code. https://github.com/sjkaliski/go-github-actions

I tried to add lint here https://github.com/CortexFoundation/torrentfs/blob/master/.github/workflows/go.yml#L32-L33 but it seems a little strict for pass. go fmt action should be OK, you can try to make a PR for it, but action link below seems better https://github.com/actions-contrib/go https://github.com/marketplace/actions/go-action Market

ucwong avatar Jul 23 '20 11:07 ucwong

Stale issue message

github-actions[bot] avatar Aug 31 '21 11:08 github-actions[bot]