Finschia / finschia-sdk

A framework for building blockchains based Finschia Mainnet that is forked from cosmos-sdk
Apache License 2.0
61 stars 31 forks source link

fix: update swap keys for possibly overlapped keys #1365

Closed jaeseung-bae closed 6 months ago

jaeseung-bae commented 6 months ago

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 35.48387% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 69.53%. Comparing base (5e2cd7f) to head (0b9cc3c).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365/graphs/tree.svg?width=650&height=150&src=pr&token=m16qfzIPO7&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia)](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia) ```diff @@ Coverage Diff @@ ## main #1365 +/- ## ========================================== - Coverage 69.53% 69.53% -0.01% ========================================== Files 672 672 Lines 56111 56128 +17 ========================================== + Hits 39019 39027 +8 - Misses 14827 14835 +8 - Partials 2265 2266 +1 ``` | [Files](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia) | Coverage Δ | | |---|---|---| | [x/fswap/keeper/keys.go](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?src=pr&el=tree&filepath=x%2Ffswap%2Fkeeper%2Fkeys.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-eC9mc3dhcC9rZWVwZXIva2V5cy5nbw==) | `86.66% <84.61%> (-13.34%)` | :arrow_down: | | [x/fswap/types/fswap.go](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?src=pr&el=tree&filepath=x%2Ffswap%2Ftypes%2Ffswap.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-eC9mc3dhcC90eXBlcy9mc3dhcC5nbw==) | `5.00% <0.00%> (ø)` | | | [x/fswap/types/msgs.go](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?src=pr&el=tree&filepath=x%2Ffswap%2Ftypes%2Fmsgs.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-eC9mc3dhcC90eXBlcy9tc2dzLmdv) | `0.00% <0.00%> (ø)` | | | [x/fswap/keeper/grpc\_query.go](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365?src=pr&el=tree&filepath=x%2Ffswap%2Fkeeper%2Fgrpc_query.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia#diff-eC9mc3dhcC9rZWVwZXIvZ3JwY19xdWVyeS5nbw==) | `6.97% <0.00%> (-1.60%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/Finschia/finschia-sdk/pull/1365/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Finschia)
jaeseung-bae commented 6 months ago

We should prohibit denoms longer than 255.

Can i have any reason for 255 limit?

0Tech commented 6 months ago

We should prohibit denoms longer than 255.

Can i have any reason for 255 limit?

It's because our newly introduced lengthPrefix() assumes it (converts length to byte).

jaeseung-bae commented 6 months ago

We should prohibit denoms longer than 255.

Can i have any reason for 255 limit?

It's because our newly introduced lengthPrefix() assumes it (converts int to byte).

Thanks for letting me know.

I found regex for denom validation [a-zA-Z][a-zA-Z0-9/-]{2,127}. function to get key will be called after validation logic, so it's okay without length limit.

jaeseung-bae commented 6 months ago

We should prohibit denoms longer than 255.

Can i have any reason for 255 limit?

It's because our newly introduced lengthPrefix() assumes it (converts int to byte).

Thanks for letting me know.

I found regex for denom validation [a-zA-Z][a-zA-Z0-9/-]{2,127}. function to get key will be called after validation logic, so it's okay without length limit.

I got another case to call without coin validation logic. I've just made commit to fix it.