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

Change type of max_gas to i64, and remove two clones. #61

Merged
merged 2 commits into from
Dec 11, 2019

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Nov 11, 2019

I'm not sure about the specification of max_gas, but the default genesis.conf generated by tendermint has max_gas set to -1, so.

tarcieri
tarcieri previously approved these changes Nov 11, 2019
@liamsi
Copy link
Member

liamsi commented Nov 28, 2019

@yihuang do you mind redoing this with signed commits?

@yihuang
Copy link
Contributor Author

yihuang commented Nov 29, 2019

@yihuang do you mind redoing this with signed commits?

Done.

liamsi
liamsi previously approved these changes Nov 29, 2019
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Strangely only one of two commits show up as signed:

image

Can't merge to master if not all commits are signed.

@yihuang
Copy link
Contributor Author

yihuang commented Dec 5, 2019

Sorry for the delay, it's done now. @liamsi

@yihuang yihuang requested a review from liamsi December 5, 2019 12:40
liamsi
liamsi previously approved these changes Dec 10, 2019
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good work as always 👍 (and sorry for the delay on my side, too)

@liamsi
Copy link
Member

liamsi commented Dec 10, 2019

Will try again later ...
image

I'm more and more convinced we should stop requiring all commits to be signed :-/ It's causing some difficulties and provides little security.

@liamsi
Copy link
Member

liamsi commented Dec 11, 2019

Tried several times to merge this. I suspect this PR can’t be merged (although all / the two commits are signed) because master moved forward one commit in the meantime; and github can’t secretly rebase and retry but the contributor has to rebase before this can be merged. At least that's my only explanation. Maybe it's just some github hickup. Will try again tomorrow.

@tarcieri
Copy link
Contributor

github can’t secretly rebase and retry

It can, but it's been explicitly disabled...

Screen Shot 2019-12-10 at 4 15 24 PM

@liamsi
Copy link
Member

liamsi commented Dec 11, 2019

It can, but it's been explicitly disabled...

Yeah, I meant it can't anymore now that "squash and merge" is the only enabled option. Maybe we can re-enable the rebase option even if we agree on a policy of only squash merging (and let at least github do the rebasing implicitly as before).

@yihuang
Copy link
Contributor Author

yihuang commented Dec 11, 2019

I just rebased to the new master.

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM 👍

@liamsi liamsi merged commit 0b9450d into informalsystems:master Dec 11, 2019
@ebuchman
Copy link
Member

Sorry for the trouble folks - disabled the signed commits requirement. Thanks @yihuang

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

Successfully merging this pull request may close these issues.

4 participants