search

CortexFoundation / CortexTheseus

Cortex - AI on Blockchain, Official Golang implementation
https://www.cortexlabs.ai
GNU Lesser General Public License v3.0
124 stars 47 forks source link

Current code review #653

Open ucwong opened 4 years ago

ucwong commented 4 years ago

Review code by package, good to point out some questions

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

DhunterAO commented 4 years ago

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

ucwong commented 4 years ago

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

combine them as a single one ?

DhunterAO commented 4 years ago

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 commented 4 years ago

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?

ucwong commented 4 years ago

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 commented 4 years ago

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 :)

DhunterAO commented 4 years ago

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.

ucwong commented 4 years ago

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

DhunterAO commented 4 years ago

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

ucwong commented 4 years ago

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

github-actions[bot] commented 3 years ago

Stale issue message