Skip to content

accounts/abi: add ErrorById#27277

Merged
holiman merged 7 commits into
ethereum:masterfrom
chawin-a:master
May 22, 2023
Merged

accounts/abi: add ErrorById#27277
holiman merged 7 commits into
ethereum:masterfrom
chawin-a:master

Conversation

@chawin-a
Copy link
Copy Markdown
Contributor

Changes

  • Modify ID type from common.Hash to []byte in Error struct
  • Add ErrorById

Comment thread accounts/abi/abi.go Outdated
return nil, fmt.Errorf("data too short (%d bytes) for abi error lookup", len(sigdata))
}
for _, errABI := range abi.Errors {
if bytes.Equal(errABI.ID, sigdata[:4]) {
Copy link
Copy Markdown
Contributor

@fjl fjl May 16, 2023

Choose a reason for hiding this comment

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

I wonder why you changed the ID field to []byte. Might be better to:

  • change the type of the sigdata parameter to [4]byte
  • do the check like bytes.Equal(errABI.ID[:4], sigdata[:])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I implement it in the same way as MethodById did. I'll modify it if [4]byte is better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, []byte is better because I can use ErrorData as an input to find abi.Error.

@deep709
Copy link
Copy Markdown

deep709 commented May 16, 2023

Thanks all

@chawin-a chawin-a requested a review from fjl May 17, 2023 06:44
@fjl
Copy link
Copy Markdown
Contributor

fjl commented May 17, 2023

Please apply the requested changes!

@chawin-a
Copy link
Copy Markdown
Contributor Author

Done please review the changes

Copy link
Copy Markdown
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Sorry some more changes needed

Comment thread accounts/abi/error.go Outdated
// ID returns the canonical representation of the error's signature used by the
// abi definition to identify event names and types.
ID common.Hash
ID [4]byte
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to change the ID to make the function ErrorByID work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed ID back to common.Hash.

Comment thread accounts/abi/abi.go Outdated

// ErrorById looks up an error by the 4-byte id,
// returns nil if none found.
func (abi *ABI) ErrorById(sigdata [4]byte) (*Error, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please rename this method to ErrorByID

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@chawin-a
Copy link
Copy Markdown
Contributor Author

Please review the changes again.

@chawin-a chawin-a requested a review from fjl May 18, 2023 04:26
Comment thread accounts/abi/abi_test.go Outdated
}
for name, m := range abi.Errors {
a := fmt.Sprintf("%v", &m)
id := *(*[4]byte)(m.ID[:4])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you should do something like this:

var id [4]byte
copy(id[:], m.ID[:4])

Thanks for submitting this!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for review

Copy link
Copy Markdown
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.11.7 milestone May 22, 2023
Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman merged commit 5b792e0 into ethereum:master May 22, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025
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.

5 participants