-
Notifications
You must be signed in to change notification settings - Fork 332
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
feat(evmutil): Add DeployedCosmosCoinContracts query #1605
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
4721ab4
add QueryDeployedCosmosCoinContracts proto
pirtleshell 796d992
setup iteration & collection for deployed addrs
pirtleshell 152729a
rewrite grpc query tests
pirtleshell 52b1c1f
support querying for all deployed contracts
pirtleshell 13eb879
support querying by cosmos denom
pirtleshell 9a7f7a2
fix & test pagination
pirtleshell 0bfef0f
remove unused iteration methods
pirtleshell 2ff9996
add CLI query command
pirtleshell 891ee23
update changelog
pirtleshell 614f5ff
update spec
pirtleshell 1f002ab
add InternalEVMAddress.MarshalJSON test
pirtleshell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice glad this is limited and tested. This is a user parameter that causes a db lookup, so without a limit this endpoint could be used for a denial of service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation is an interesting problem. It feels like there are cases where a range & filter would be superior over direct lookups, but definitely not for small numbers of filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, range & filter would allow pagination on this endpoint when requested with a >100 denoms query, too. but i made the tradeoff with the assumption that the most common use cases were either
i think the majority of the time, 2) is going to be more efficient with direct lookups. a larger factor was that this split between a method that paginates all results & a method that only does direct lookups on a list felt easier to read in the code than the single PaginateFilter that handled both cases.
a nice bonus is that denoms that exist are returned in the order they were requested in (though omitting missing denoms mean there's no guarantee that search index == result index)