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

Make query.FilteredPaginate use generics, and remove the accumulate parameter #12242

Closed
4 tasks
ValarDragon opened this issue Jun 14, 2022 · 1 comment · Fixed by #12253
Closed
4 tasks

Make query.FilteredPaginate use generics, and remove the accumulate parameter #12242

ValarDragon opened this issue Jun 14, 2022 · 1 comment · Fixed by #12253
Assignees

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 14, 2022

Summary

Currently query.FilteredPaginate creates a silly developer overhead for callers, as they have to maintain the entire slice to return at the end. (and handle the complexity for the accumulate parameter.

Ideally this is instead handled by generic routines, and not the developer writing the query. This previously wasn't cleanly doable, due to needing to maintain the return type callee side. But now that generics have landed, the filtered paginate queries can easily use a generic type parameter for the slice type. Furthermore lets make this parameter a proto.message, so the serialization / deserialization can be handled for the caller.

Problem Definition

Reduce the developer overhead of writing a filtered paginate query. Currently even the simplest ones, net around 40 lines for the caller.

Proposal

Change filtered paginate from having the caller have to define the list of whats accumulated.

This should change the following API

var authorizations []*authz.Grant
pageRes, err := query.FilteredPaginate(grantsStore, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) {
auth, err := unmarshalAuthorization(k.cdc, value)
if err != nil {
return false, err
}
auth1, err := auth.GetAuthorization()
if err != nil {
return false, err
}
if accumulate {
msg, ok := auth1.(proto.Message)
if !ok {
return false, status.Errorf(codes.Internal, "can't protomarshal %T", msg)
}
authorizationAny, err := codectypes.NewAnyWithValue(msg)
if err != nil {
return false, status.Errorf(codes.Internal, err.Error())
}
authorizations = append(authorizations, &authz.Grant{
Authorization: authorizationAny,
Expiration: auth.Expiration,
})
}
return true, nil
})
if err != nil {
return nil, err
}
return &authz.QueryGrantsResponse{
Grants: authorizations,
Pagination: pageRes,
}, nil
to

	return query.FilteredPaginate[Authorization](grantsStore, req.Pagination, k.cdc, func(key []byte, auth Authorization) (Authorization, error) {
		auth1, err := auth.GetAuthorization()
		if err != nil {
			return false, err
		}

		// (This ok check seems useless even in he original code)	
		msg, ok := auth1.(proto.Message)
		if !ok {
			return false, status.Errorf(codes.Internal, "can't protomarshal %T", msg)
		}

		authorizationAny, err := codectypes.NewAnyWithValue(msg)
		if err != nil {
			return false, status.Errorf(codes.Internal, err.Error())
		}
	
		return authorizationAny, nil
	})

Perhaps the new function needs a new name for backwards API compatibility? I'm not too sure though, I feel like this is a code reduction that should be universally accepted here.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

@facundomedica 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants