Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Current code review #653

Open
ucwong opened this issue Jul 4, 2020 · 11 comments
Open

Current code review #653

ucwong opened this issue Jul 4, 2020 · 11 comments

Comments

@ucwong
Copy link
Member

ucwong commented Jul 4, 2020

Review code by package, good to point out some questions

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

@DhunterAO
Copy link
Contributor

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

@ucwong
Copy link
Member Author

ucwong commented Jul 4, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

ucwong commented Jul 4, 2020

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
Copy link
Member Author

ucwong commented Jul 4, 2020

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
Copy link
Contributor

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
Copy link
Member Author

ucwong commented Jul 4, 2020

#654 Switch quota to uint64

@ShadowErii ShadowErii linked a pull request Jul 20, 2020 that will close this issue
@ShadowErii ShadowErii removed a link to a pull request Jul 20, 2020
@DhunterAO
Copy link
Contributor

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

@ucwong
Copy link
Member Author

ucwong commented Jul 23, 2020

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
Copy link

Stale issue message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants