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 Authz simulation code #2170

Closed
1 task
ValarDragon opened this issue Jul 20, 2022 · 1 comment · Fixed by osmosis-labs/cosmos-sdk#292 or #2185
Closed
1 task

Fix Authz simulation code #2170

ValarDragon opened this issue Jul 20, 2022 · 1 comment · Fixed by osmosis-labs/cosmos-sdk#292 or #2185
Assignees

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Jul 20, 2022

Background

Found by @czarcas7ic , just capturing DM;s into an issue. Long simulation runs will currently fail due to errors in the Authz simulation code. All simulation runs in the SDK were only tested with usage of a single denom. As we test many denoms, we have surfaced this error.

Suggested Design

The bug is due to the terrible API design of the sdk.Coins object and its comparison definitions.
Authz SimulateGrantExec does

		if sendAuth.SpendLimit.IsAllLTE(coins) {

but it actually needs to be

		if sendAuth.SpendLimit.IsAllLTE(coins) && coins.DenomsSubsetOf(sendAuth.SpendLimit)

because what happens if you select a coin thats not in the spendlimit denoms. (One would sanely expect the old code to work, alas the API) Maybe fixed in the future if the bikeshed around better API's converges: cosmos/cosmos-sdk#11223

For now we should fix this in our SDK fork.

Acceptance Criteria

  • Long simulation runs no longer fail due to Authz simulation bug
@czarcas7ic
Copy link
Member

I am just going to reopen this until I update the sdk fork we are using so I dont forget!

@czarcas7ic czarcas7ic reopened this Jul 21, 2022
Repository owner moved this from Done ✅ to In Progress🏃 in Osmosis Chain Development Jul 21, 2022
Repository owner moved this from In Progress🏃 to Done ✅ in Osmosis Chain Development Jul 22, 2022
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
2 participants