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

New Transaction Format #223

Merged
merged 5 commits into from Dec 16, 2017

Conversation

Projects
None yet
2 participants
@jannikluhn
Contributor

jannikluhn commented Dec 14, 2017

This adds the sharding transaction class. Initially I tried to copy and adapt vm/forks/frontier/* as well, but at some point I realized that this is useless as everything would have to be rewritten as part of the account abstraction PR anyway. So this PR remains fairly basic.

One test is x-failing because of a bug in pyRLP, I'm gonna fix that over there.

@jannikluhn

This comment has been minimized.

Show comment
Hide comment
@jannikluhn

jannikluhn Dec 14, 2017

Contributor

Wait, that's not a bug in pyRLP, I'm just confused by too many brackets. Will fix tomorrow.

Contributor

jannikluhn commented Dec 14, 2017

Wait, that's not a bug in pyRLP, I'm just confused by too many brackets. Will fix tomorrow.

@pipermerriam

One suggested cleanup, otherwise I think this looks good.

super().__init__(Binary(max_length=32))
def serialize(self, obj):
result = super().serialize(obj)

This comment has been minimized.

@pipermerriam

pipermerriam Dec 15, 2017

Member

Can we move this to below the if/else so that it only happens if the validation checks pass.

@pipermerriam

pipermerriam Dec 15, 2017

Member

Can we move this to below the if/else so that it only happens if the validation checks pass.

This comment has been minimized.

@jannikluhn

jannikluhn Dec 15, 2017

Contributor

I'd prefer to leave it there, as super().serialize(obj) does a lot of checks already. In particular, that obj is actually a list and obj[0] is binary. If that weren't the case, the error messages would be misleading.

@jannikluhn

jannikluhn Dec 15, 2017

Contributor

I'd prefer to leave it there, as super().serialize(obj) does a lot of checks already. In particular, that obj is actually a list and obj[0] is binary. If that weren't the case, the error messages would be misleading.

This comment has been minimized.

@pipermerriam

pipermerriam Dec 15, 2017

Member

That's fine. I wasn't aware of the checks that occur in the super call.

@pipermerriam

pipermerriam Dec 15, 2017

Member

That's fine. I wasn't aware of the checks that occur in the super call.

@pipermerriam

This comment has been minimized.

Show comment
Hide comment
@pipermerriam
Member

pipermerriam commented Dec 15, 2017

:shipit:

jannikluhn added some commits Dec 14, 2017

@jannikluhn jannikluhn merged commit 0b9e4fb into ethereum:sharding Dec 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jannikluhn jannikluhn referenced this pull request Dec 16, 2017

Closed

STUB: New transaction format #194

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment