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

fix: log unknown actor type without returning error #1169

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

Terryhung
Copy link
Collaborator

Don't error on unknown method numbers for FEVM.

Due to the previous fix, it will return the Unknown method, then cause the error in parsed_message. Therefore, we can use the same logic as vm_message: logging the error without returning error.

@Terryhung Terryhung marked this pull request as ready for review March 21, 2023 02:14
@@ -185,7 +185,8 @@ func MethodAndParamsForMessage(m *types.Message, destCode cid.Cid) (string, stri

params, method, err := ParseParams(m.Params, m.Method, destCode)
if method == "Unknown" {
return "", "", fmt.Errorf("unknown method for actor type %s: %d", destCode.String(), int64(m.Method))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the correct behavior if a method is unknown. I don't think we can write empty strings to models that expect a method name. One option would be to remove returning the name of the method being called here, and instead post process tables needing a method name string. cc @birdychang

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 just put ‘unknown’ as the method name?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always post process later. Also, don’t we wanna deprecate parsed messages? I don’t really care about the model. Let’s make it work first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just store the method code as the method name here. There’s should be separate columns in case it fails to lookup the name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Store the method in string format.

@Terryhung Terryhung merged commit 427dab1 into master Mar 22, 2023
@Terryhung Terryhung deleted the terryhung/skip-unknow-actor-type branch March 22, 2023 05:16
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.

3 participants