-
Notifications
You must be signed in to change notification settings - Fork 589
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
tokenfactory: address mutation tests part 2 #2504
Conversation
x/tokenfactory/types/denoms.go
Outdated
@@ -56,7 +56,7 @@ func DeconstructDenom(denom string) (creator string, subdenom string, err error) | |||
} | |||
|
|||
creator = strParts[1] | |||
_, err = sdk.AccAddressFromBech32(creator) | |||
addr, err := sdk.AccAddressFromBech32(creator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done to make mutation tests happy. Understood that it is the same as what it replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
x/tokenfactory/types/denoms.go
Outdated
@@ -66,7 +66,7 @@ func DeconstructDenom(denom string) (creator string, subdenom string, err error) | |||
// So we have to join [2:] with a "/" as the delimiter to get back the correct subdenom which should be "atomderivative/sikka" | |||
subdenom = strings.Join(strParts[2:], "/") | |||
|
|||
return creator, subdenom, nil | |||
return addr.String(), subdenom, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious - what difference does this make for mutation testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this change is unclear to me. We can rename it as "creatorAddr" if we want to, but this generally seems odd to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it is odd. We are using the sdk.AccAddressFromBech32
just as a validation check. When mutation tests nil line 59 out, we have no checks that actually utilize this value since we are only using it as a check. By utilizing its output, mutation tests will then pass since we are now relying on the function call. I can change it back, but feel that since it does the same thing and now the mutation passes, it should be a non issue. Lmk what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrmm, this particular instance seems like a non-issue, but I would like to get mutation testing fixed to not make this error in the future.
This is good to merge rn though! Nice work
Nice work! |
Closes: #2105
What is the purpose of the change
Brings mutation tests for tokenfactory from 37% to 75% passing.
Brief Changelog
Testing and Verifying
This change is already covered by existing tests
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no