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.

Sign up for GitHub

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

Jump to bottom

accounts/abi: fix panic in MethodById lookup. Fixes #17797 #17798

Merged
merged 1 commit into from
Oct 1, 2018
Merged

accounts/abi: fix panic in MethodById lookup. Fixes #17797 #17798

merged 1 commit into from
Oct 1, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Oct 1, 2018

Trivial fix,

@holiman holiman requested a review from gballet October 1, 2018 08:37
gballet
gballet reviewed Oct 1, 2018
View reviewed changes
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor code style commment.

accounts/abi/abi.go
@@ -137,6 +137,9 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {
// MethodById looks up a method by the 4-byte id
// returns nil if none found
func (abi *ABI) MethodById(sigdata []byte) (*Method, error) {
if len(sigdata) < 4 {
Copy link
Member

Choose a reason for hiding this comment

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

Please use a constant name instead of a hardcoded number, something like MethodIdLength

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahmm.. really? MethodById looks up a method by the 4-byte id -- it's a 4-byte id, do we really have to make it generic for N-byte ids?

Copy link
Member

Choose a reason for hiding this comment

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

it's not about being generic (a variable) but documenting (a constant name). Alright, it's not a blocker let's move on.

accounts/abi/abi_test.go Outdated
}
}

func TestABI_MethodByIdEmpty(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is a leftover from a previous test, and that you moved the test to TestABI_MethodById ? Anyhow, no need to commit an empty function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops

@holiman
Copy link
Contributor Author

holiman commented Oct 1, 2018

Fixed, squashed

gballet
gballet approved these changes Oct 1, 2018
View reviewed changes
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@gballet gballet merged commit 96fd50b into ethereum:master Oct 1, 2018
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
1f8e20c
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
f905fce
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
b249ee1
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
5b2eecf
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
8e94d0e
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
f3a2109
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
7678ad3
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 27, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
85a6c0a
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 28, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
71e8920
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 30, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
45e4505
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 30, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
661d79a
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 30, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
7078f65
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 30, 2024
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
aafa78b
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 1, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
4baeb5b
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 3, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
dea4223
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 4, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
703db54
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 6, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
c1d4bca
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
3049900
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 10, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
2dce490
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 14, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
59061ba
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 16, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
5b368c6
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 17, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
bafae24
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 22, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
c338d2b
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 22, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
98a15a1
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 23, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
d68f23a
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025
@gzliudan
accounts/abi: fix panic in MethodById lookup (ethereum#17798)
e2ea9e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gballet gballet gballet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@holiman @gballet