x/incentives: cli updates

[czarcas7ic]

Part of: #1837

What is the purpose of the change

Audit, clean up, and test modifications for the incentives module

This is part 2 of 4:

1. Proto changes
2. CLI fixes
3. Tests
4. All other changes

Brief Changelog

• Fixes reward estimation query to work as intended (pulls owners lockIDs if not provided)
• Improve / cleanup CLI tests

Testing and Verifying

This change is already covered by existing tests, such as cli_test.go within the incentives module

Documentation and Release Note

• Does this pull request introduce a new feature or user-facing behavior changes? no
• Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
• How is the feature or change documented? not applicable

[czarcas7ic]

Still having the issue mentioned here

For some addresses the rewards estimation is fine and others it panics.

Here is a random one I found on mintscan as a panic example:
osmosisd q incentives rewards-estimation --owner osmo1lqxwdzlxvvh700sxagcm5flw2tpmp890s76lsj --end-epoch 400

Using this branch will give you a panic (using main wont work at all since this query has been broken, I modified the query in this PR to try to get it to work as intended. When presented with just an address and no explicit lock-ids this command was supposed to utilize all locks the user has that are active, but that is not the behavior on main)

You can use osmosisd q lockup account-locked-longer-duration osmo1lqxwdzlxvvh700sxagcm5flw2tpmp890s76lsj 1s to see all the lock IDs and then query the rewards estimation using the lock-ids flag for each individual lock to see which locks cause this panic, but I have yet to see a clear pattern to which locks cause panics and which dont.

This address though, with multiple locks, gives a valid response:
osmosisd q incentives rewards-estimation --owner osmo13gx9q6q8cxhym9pzyvjtkgagjqrgwxe0gcleyh --end-epoch 400

EDIT: Fixed as far as I can without implementing accumulation store fix

[p0mvn]

I don't have a clear suggestion but I'm wondering if it is possible to decouple the command logic from the flags provided. In my opinion, having so many edge cases makes it likely for something wrong to happen

[mattverse]

I suggest we not merge this until we fix the bug in GetCmdUpcomingGaugesPerDenom

[mattverse]

You can use osmosisd q lockup account-locked-longer-duration osmo1lqxwdzlxvvh700sxagcm5flw2tpmp890s76lsj 1s to see all the lock IDs and then query the rewards estimation using the lock-ids flag for each individual lock to see which locks cause this panic, but I have yet to see a clear pattern to which locks cause panics and which dont.

Can you try running binary with --debug to see the specific error / panic message?

[czarcas7ic]

@mattverse running with debug logs still provides the same panic message:

Error: rpc error: code = Unknown desc = panic message redacted to hide potentially sensitive system info: panic

I wish we didn't have this panic message, it makes debugging so difficult -_-

[czarcas7ic]

I suggest we not merge this until we fix the bug in GetCmdUpcomingGaugesPerDenom

Which bug are you referring to?

[ValarDragon]

comment updates LGTM, and the GetRewardsEst logic. (beyond the comment I left)

Lets capture the remaining mystery problem there into an issue that we add to a backlog?

Didn't review cli_test changes in detail, but I'm also generally not concerned with the cli_tests

[czarcas7ic]

According to Dev, pools that existed prior to v4 or v5 have an incorrect/negative accumulation store value. After porting the redacted error fix from the sdk, I was able to see that the accumulation store was in fact returning a negative number which was causing the error. Looking into either fixing this or merging this as is and fixing this problem in a future PR

[czarcas7ic]

Made it so that the est rewards query will always return a valid result and will show an error like the following within the daemon if a bugged gauge is part of the request

Also added logic to grpc_query.go to allow for blank owner address unlike before. Please let me know if this matches the intent, if so I will merge!

[ValarDragon]

Awesome, thanks!

[czarcas7ic]

Backport this