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 handling of --page-key value to expect a base64 encoded string #332

Closed
4 tasks
dwedul-figure opened this issue May 22, 2021 · 0 comments · Fixed by #390
Closed
4 tasks

Fix handling of --page-key value to expect a base64 encoded string #332

dwedul-figure opened this issue May 22, 2021 · 0 comments · Fixed by #390
Assignees
Labels
bug Something isn't working CLI Command line interface feature
Projects
Milestone

Comments

@dwedul-figure
Copy link
Contributor

Summary of Bug

With pagination, there's a next_key value that is a byte array. The CLI outputs byte arrays as base64 encoded values.

However, the ReadPageRequest function converts the provided --page-key string to a byte array by casting it: []byte(pageKey). That means that if you copy a next_key from a result and provide it as the --page-key in a new request, you get unexpected result. The resulting key in the request wasn't base64 decoded, so it's pretty close to just a random value.

Version

1.3.0

More Details

Anywhere we are calling the ReadPageRequest function, we need to base64 decode the --page-key string first.

From cosmwasm:

// sdk ReadPageRequest expects binary but we encoded to base64 in our marshaller
func withPageKeyDecoded(flagSet *flag.FlagSet) *flag.FlagSet {
	encoded, err := flagSet.GetString(flags.FlagPageKey)
	if err != nil {
		panic(err.Error())
	}
	raw, err := base64.StdEncoding.DecodeString(encoded)
	if err != nil {
		panic(err.Error())
	}
	flagSet.Set(flags.FlagPageKey, string(raw))
	return flagSet
}

We need to create a similar function for our CLI stuff to use. Then, wherever we call ReadPageRequest(cmd.Flags()) we would change that to ReadPageRequest(withPageKeyDecoded(cmd.Flags())).

Here's currently all the places that need updating.

> grep -Irsn --exclude-dir vendor ReadPageRequest *
x/attribute/client/cli/query.go:93:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/attribute/client/cli/query.go:141:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/attribute/client/cli/query.go:187:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/marker/client/cli/query.go:85:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/marker/client/cli/query.go:131:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/name/client/cli/query.go:133:			pageReq, err := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:613:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:669:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:724:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:818:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:869:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:944:	pageReq, e := client.ReadPageRequest(cmd.Flags())
x/metadata/client/cli/query.go:1058:	pageReq, e := client.ReadPageRequest(cmd.Flags())

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@dwedul-figure dwedul-figure added bug Something isn't working CLI Command line interface feature labels May 22, 2021
@iramiller iramiller added this to Triage in Mainnet via automation Jun 15, 2021
@iramiller iramiller moved this from Triage to To Do in Mainnet Jun 15, 2021
@iramiller iramiller added this to the v1.6.0 milestone Jun 23, 2021
@iramiller iramiller moved this from To Do to Backlog in Mainnet Jun 24, 2021
@iramiller iramiller moved this from Backlog to To Do in Mainnet Jul 19, 2021
@dwedul-figure dwedul-figure self-assigned this Jul 27, 2021
@dwedul-figure dwedul-figure moved this from To Do to In Progress in Mainnet Jul 30, 2021
Mainnet automation moved this from In Progress to Done Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI Command line interface feature
Projects
Mainnet
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants