fix(claimsmanager): properly check index to avoid invalid data in UserClaims,UserLastEpochClaims #1234
Conversation
odeke-em
requested review from
joe-bowman,
faddat,
minhngoc274 and
tuantran1702
as code owners
|
@odeke-em is attempting to deploy a commit to the joe-bowman's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe recent update focuses on enhancing the robustness of the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
…rClaims,UserLastEpochClaims This change ensures that we correctly check for missing values in keeper.(UserClaims, UserLastEpochClaims) because otherwise due to the blind assumption that the key would always be well formed, +1 and +1+1 were being added respectively which would mean that the condition "idx >= 0" would ALWAYS pass and send over the wrong data if the key happened to contain the address bytes. Fixes quicksilver-zone#1217 Supersedes PR quicksilver-zone#1223
odeke-em
force-pushed
the
x-claimsmanager-keeper-properly-skip-over-negative-indices
branch
from
e024aed to
6f8f1c2
Compare
|
/cc @joe-bowman @faddat @minhngoc274 please see this PR which fixes the issue. Thank you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/claimsmanager/keeper/grpc_query.go (2 hunks)
Additional comments: 2
x/claimsmanager/keeper/grpc_query.go (2)
- 72-80: The logic for calculating the index to check for the presence of address bytes in the key within the
UserClaimsfunction has been updated. While the adjustment to skip the first prefix byte (0x00) and the separator is correct, there are a few considerations:
- Ensure that the length of
keyis always greater thanidx+len(addrBytes)to avoid potential index out of bounds errors.- The addition of
1 + 1in line 78 is correct in this context, as it accounts for the initial byte and the separator. However, adding a brief comment explaining why2is added (to skip the prefix and the separator) could enhance code readability.However, consider adding a safety check for the key length and improving the comment for clarity.
- 95-103: In the
UserLastEpochClaimsfunction, the logic for identifying the index of address bytes within the key has been modified. This change correctly accounts for the first byte being 0x01 and skips the separator withidx += 1. Similar considerations apply here:
- It's crucial to ensure that the length of
keyis sufficient to prevent index out of bounds errors when accessingkey[idx:idx+len(addrBytes)].- The rationale behind the index calculation and the assumption about the key structure could benefit from more detailed comments for future maintainability.
Recommend adding a length check for
keyand enhancing comments for better understanding.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/claimsmanager/keeper/grpc_query.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/claimsmanager/keeper/grpc_query.go
odeke-em
deleted the
x-claimsmanager-keeper-properly-skip-over-negative-indices
branch
This change ensures that we correctly check for missing values in keeper.(UserClaims, UserLastEpochClaims) because otherwise due to the blind assumption that the key would always be well formed, +1 and +1+1 were being added respectively which would mean that the condition "idx >= 0" would ALWAYS pass and send over the wrong data if the key happened to contain the address bytes.
Fixes #1217
Supersedes PR #1223
Summary by CodeRabbit