-
Notifications
You must be signed in to change notification settings - Fork 370
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
Conversation
d2ac4a1
to
6ad549e
Compare
29da93f
to
c617be1
Compare
* also implement protobuf marshaler for InternalEVMAddress
c617be1
to
614f5ff
Compare
func getDeployedCosmosCoinContractsByDenoms( | ||
k *Keeper, ctx sdk.Context, denoms []string, | ||
) (*types.QueryDeployedCosmosCoinContractsResponse, error) { | ||
if len(denoms) > query.DefaultLimit { |
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
- looking up all contract addresses (no denoms in query)
- looking up contract address for only a handful of contracts (limited number of denoms in query)
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)
Description
this PR isn't as big as it looks, i promise! 😂 its ~200 lines of actual code and 250 lines of tests. the rest is proto generated things.
There's no error when empty:
Here's the response with data:
Checklist