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

EEI: Specify signness #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

EEI: Specify signness #134

wants to merge 1 commit into from

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Aug 21, 2018

Resolves #132.

@axic
Copy link
Member

axic commented Aug 22, 2018

Can you split this into two PRs? One codifies what is happening in Hera right now, the other one changes to signed types.

@chfast
Copy link
Collaborator Author

chfast commented Aug 22, 2018

Looks like a lot of unnecessary work.

@jakelang jakelang requested a review from axic September 16, 2018 01:50
@axic
Copy link
Member

axic commented Sep 19, 2018

Looks like a lot of unnecessary work.

Yeah but it would be nice to merge what's going on right now, and then we can review the proposed changes and reason about them,

@axic axic mentioned this pull request Oct 7, 2018
@axic axic force-pushed the eei-exceptions branch 2 times, most recently from 5879f4d to 297701e Compare October 7, 2018 22:07
@axic axic changed the title EEI: Specify signness and trap conditions EEI: Specify signness Oct 7, 2018
@axic
Copy link
Member

axic commented Oct 18, 2018

Needs to be rebased.

We also need to make a decision about this and implement it in Hera.

@jakelang @poemm @gballet can you please review?

@poemm
Copy link
Contributor

poemm commented Oct 18, 2018

I looked through each edit. I see no reason for any of these arguments to be signed. It just adds complexity, adds checking overhead, and decreases possible address offsets, lengths, indices, and gas by a factor of approximately two.

I vote to interpret all of these arguments as unsigned integers. If a contract makes an error and uses a number which is negative in 2's complement, then they will probably quickly run out of gas.

@axic
Copy link
Member

axic commented Oct 18, 2018

There seems to be strong opinions on this one. @chfast can you reflect on @poemm's points?

Do you think we can make a resolution in the next few days? If not, we have to move this off "Revision 4". Would like to close "Revision 4" before Monday.

@gballet
Copy link
Member

gballet commented Oct 21, 2018

Agreed with @poemm , already had to deal with nasty sign conversion bugs in the EEI and I don't want the extra complexity. Plus, modifying the i32ptr type is going to reduce the available memory from 4G down to 2G and I'm sure that's going to impact turbo-ewasm and wasmOS in the not too distant future.

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

Successfully merging this pull request may close these issues.

4 participants