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

Fixed typos in abi-spec documentation #3386

Merged
merged 2 commits into from
May 4, 2018

Conversation

nisdas
Copy link
Contributor

@nisdas nisdas commented Jan 7, 2018

No description provided.

@chriseth
Copy link
Contributor

chriseth commented Jan 7, 2018

@nisdas I took the liberty to explain the components of X a little better. Also, I think that k >= 0 is intentional to allow zero-length tuples.

@chriseth chriseth requested a review from axic January 7, 2018 12:56
@chriseth
Copy link
Contributor

@axic, please review

@@ -97,7 +97,7 @@ We distinguish static and dynamic types. Static types are encoded in-place and d
* ``bytes``
* ``string``
* ``T[]`` for any ``T``
* ``T[k]`` for any dynamic ``T`` and any ``k > 0``
* ``T[k]`` for any dynamic ``T`` and any ``k >= 0``
Copy link
Member

Choose a reason for hiding this comment

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

Why is T[0] valid in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? It reduces the amount of exceptional cases.

@chriseth
Copy link
Contributor

@axic to merge or not to merge? I'm fine with disallowing k==0 if you insist.

@axic
Copy link
Member

axic commented May 4, 2018

@axic to merge or not to merge? I'm fine with disallowing k==0 if you insist.

I don't mind. T[0] feels useless though.

@chriseth
Copy link
Contributor

chriseth commented May 4, 2018

Will rebase.

@chriseth chriseth merged commit 8d666e1 into ethereum:develop May 4, 2018
@axic
Copy link
Member

axic commented May 4, 2018

So we went for allowing a useless construct of empty arrays? :)

@axic
Copy link
Member

axic commented May 4, 2018

Probably it made sense merging this as is because it matches the code, but I think it should be discussed for both tuples and fixed-length arrays. Created an issue for that to receive more input.

@chriseth
Copy link
Contributor

chriseth commented May 4, 2018

Actually I realized that we need empty statically-sized arrays: Otherwise, it is undefined how to encode a dynamic array of length zero.

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.

None yet

3 participants