chore: Change Query/DenomTrace gRPC #1312
Conversation
catShaark
requested review from
colin-axner,
fedekunze,
AdityaSripal,
crodriguezvega,
seantking,
charleenfei and
damiannolan
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1312 +/- ##
==========================================
- Coverage 80.27% 80.25% -0.03%
==========================================
Files 166 166
Lines 12023 12026 +3
==========================================
Hits 9652 9652
- Misses 1916 1918 +2
- Partials 455 456 +1
|
There was a problem hiding this comment.
Thanks, @catShaark. I left just a couple of nits. And we need a changelog entry. :) Since this changing the API, then I would say in the API breaking section.
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
catShaark
changed the title
| @@ -21,10 +21,10 @@ func (suite *KeeperTestSuite) TestQueryDenomTrace() { | |||
| expPass bool | |||
| }{ | |||
| { | |||
| "invalid hex hash", | |||
| "invalid ibc denom", | |||
There was a problem hiding this comment.
nit: could we put the success case first? :=)
| if !strings.HasPrefix(req.Denom, "ibc/") { | ||
| return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("invalid denom with no ibc prefix: %s", req.Denom)) | ||
| } |
There was a problem hiding this comment.
I think we should make this change backwards compatible. We could do so by checking if it has the ibc prefix and then remove the prefix before performing the query. I should have specified this on the original issue (but at the time there was a lot less dependencies on this gRPC
So essentially:
if strings.HasPrefix(denom, "ibc/") {
denom = denom[4:]
}There was a problem hiding this comment.
If so, how should we name the field in the gRPC request, is it still denom, now that the field means denom or hash
There was a problem hiding this comment.
So I'll make it like you said and leave the field name still be denom
There was a problem hiding this comment.
That's a good suggestion from @colin-axner. If you make the change backwards compatible, then the changelog entry shouldn't be in the API breaking section, but in the improvements one instead.
There was a problem hiding this comment.
I'll apply the change accordingly and I'll change the field back to hash
|
I'll close this PR and make a new one #1342 |
Description
Change the
DenomTracegrpc so that it takes the whole ibc denom instead of just the hashcloses: #101
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes