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

authz queries to meet the expanded types provided in the sdk #1308

Merged
merged 15 commits into from
Jan 3, 2023

Conversation

mvid
Copy link
Contributor

@mvid mvid commented Oct 25, 2022

No description provided.

@mvid
Copy link
Contributor Author

mvid commented Oct 25, 2022

This PR is split from #1307

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff looks good to me, thank you!

Could you add tests for the new queries to packages/stargate/src/modules/authz/queries.spec.ts? There you already see a test for the grants query which I guess can easily be copied and adapted.

@webmaster128
Copy link
Member

I re-opened to try to trigger a CI run. Not sure what kind of problem CircleCI has here. Maybe a few more commits are sufficient, let's see.

@mvid
Copy link
Contributor Author

mvid commented Nov 26, 2022

@webmaster128 Took some tries to get the linter/tests to work, since I am in Intellij which isn't well supported in this setup, but I believe this is now ready!

@mvid
Copy link
Contributor Author

mvid commented Dec 2, 2022

@webmaster128 Any more notes?

@webmaster128 webmaster128 added this to the 0.30.0 milestone Dec 7, 2022
@mvid
Copy link
Contributor Author

mvid commented Jan 2, 2023

Pinging this for an update

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you.

Just one point: could you make one describe block per method? I.e. add the tests on the same level as describe("grants", () => {. This allows us to add multiple tests per method (not needed here).

});

it("works querying by granter", async () => {
pendingWithoutSimapp46();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in cosmos-sdk 0.45.2 👍

@mvid
Copy link
Contributor Author

mvid commented Jan 3, 2023

@webmaster128 done

@webmaster128
Copy link
Member

Nice, thank you! Could you enable modification for maintainers? I rebased this to latest main and added a CHANGELOG entry to get it merged.

@mvid
Copy link
Contributor Author

mvid commented Jan 3, 2023

@webmaster128 That is odd... I already have it set to allow edits by maintainers

Screenshot 2023-01-03 at 15 36 43

@webmaster128
Copy link
Member

Hmm, very strange. Don't know. In this case could you rebase to latest main (or merge main here) and add this CHANGELOG entry to the Unreleased secition?

### Added

- @cosmjs/stargate: Add `granteeGrants` and `granterGrants` queries to
  `AuthzExtension` ([#1308]).

[#1308]: https://github.com/cosmos/cosmjs/pull/1308

@mvid
Copy link
Contributor Author

mvid commented Jan 3, 2023

Done, pulled main and added the log entry

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@webmaster128 webmaster128 merged commit 33271bc into cosmos:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants