[TODO] chore: update smt.MerkleRoot#Sum() error handling
#672
Conversation
bryanchriswhite
added
bug
WalkthroughThis update primarily refactors cryptographic operations and Merkle tree handling across various files. Key changes include replacing Changes
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 Configuration File (
|
bryanchriswhite
force-pushed
the
issues/584/refactor/smt-root
branch
from
2f0863b
to
fe59bcb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/proof/types/claim.go (1 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (5 hunks)
Additional comments not posted (4)
x/proof/types/claim.go (2)
26-26: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetNumComputeUnitshandle the potential error correctly.Verification successful
Verification successful: Error handling for
GetNumComputeUnitsis correctly implemented.The error handling for all calls to the
GetNumComputeUnitsfunction is properly implemented. No further changes are necessary.
x/tokenomics/keeper/settle_pending_claims.gox/proof/keeper/msg_server_submit_proof.gox/proof/keeper/msg_server_create_claim.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetNumComputeUnits` handle the potential error correctly. # Test: Search for the function usage. Expect: Proper error handling. rg --type go -A 5 $'GetNumComputeUnits()'Length of output: 9509
Line range hint
35-35:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetNumRelayshandle the potential error correctly.Verification successful
Ensure all function calls to
GetNumRelayshandle errors correctly.The function calls to
GetNumRelaysin the following files handle errors appropriately:
x/tokenomics/keeper/settle_pending_claims.gox/proof/keeper/msg_server_submit_proof.gox/proof/keeper/msg_server_create_claim.goThese instances show that the error returned by
GetNumRelaysis properly checked and handled.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetNumRelays` handle the potential error correctly. # Test: Search for the function usage. Expect: Proper error handling. rg --type go -A 5 $'GetNumRelays()'Length of output: 8337
x/tokenomics/keeper/settle_session_accounting.go (2)
30-30: LGTM! The error handling updates are appropriate.The code changes are approved.
182-182: LGTM! The error handling updates are appropriate.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- go.mod
bryanchriswhite
added
push-image
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 672) |
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.mod (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- go.mod
bryanchriswhite
removed
push-image
…-root * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- go.mod (1 hunks)
- testutil/proof/fixture_generators.go (2 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (5 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- go.mod
- testutil/proof/fixture_generators.go
- x/tokenomics/keeper/settle_session_accounting.go
- x/tokenomics/keeper/settle_session_accounting_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.mod (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/tokenomics/keeper/settle_session_accounting.go (5 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_session_accounting_test.go
Additional context used
GitHub Check: go-test
x/tokenomics/keeper/settle_session_accounting.go
[failure] 80-80:
undefined: types) (typecheck)
[failure] 80-80:
undefined: types) (typecheck)
[failure] 80-80:
undefined: types) (typecheck)
[failure] 80-80:
undefined: types) (typecheck)
[failure] 80-80:
undefined: types) (typecheck)
Additional comments not posted (4)
x/tokenomics/keeper/settle_session_accounting.go (4)
5-5: Approved addition ofcrypto/sha256import.This import is necessary for handling digest size checks in the
SettleSessionAccountingfunction.
32-32: Approved changes to function signatures for better error handling.The modifications to include error returns in these functions are a good practice, ensuring that errors are not silently ignored and can be handled upstream.
Also applies to: 111-111
40-45: Approved robust error handling and logging enhancements.The addition of detailed error checks and logging improves the function's robustness and traceability, which is critical in a financial context.
Also applies to: 77-88
213-228: Approved changes togetCoinFromComputeUnitswith a suggestion for further validation.The changes to return an error and the new computation logic are good. However, ensure that the mathematical operations handle edge cases like overflow or underflow.
bryanchriswhite
added
push-image
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 672) |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pkg/crypto/protocol/hasher.go (1)
1-12: Consider exportingtrieRootMetadataSizeif needed.The TODO comment suggests exporting
trieRootMetadataSizefrom the SMT package. Ensure this is addressed if required.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (8)
- pkg/client/supplier/client_test.go (3 hunks)
- pkg/crypto/protocol/hasher.go (1 hunks)
- pkg/relayer/session/sessiontree.go (2 hunks)
- tests/integration/tokenomics/relay_mining_difficulty_test.go (2 hunks)
- testutil/proof/fixture_generators.go (2 hunks)
- x/application/keeper/auto_undelegate.go (1 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (6 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (4 hunks)
Files skipped from review due to trivial changes (1)
- x/application/keeper/auto_undelegate.go
Files skipped from review as they are similar to previous changes (3)
- testutil/proof/fixture_generators.go
- x/tokenomics/keeper/settle_session_accounting.go
- x/tokenomics/keeper/settle_session_accounting_test.go
Additional comments not posted (5)
pkg/client/supplier/client_test.go (2)
16-16: LGTM!The import statement for
protocolfrompkg/cryptois correct and necessary.
184-184: LGTM!The change from
sha256.New()toprotocol.NewTrieHasher()is correct and consistent with the new implementation.pkg/relayer/session/sessiontree.go (2)
14-14: LGTM!The import statement for
protocolfromgithub.com/pokt-network/poktroll/pkg/crypto/protocolis correct and necessary.
89-89: LGTM!The change from
sha256.New()toprotocol.NewTrieHasher()is correct and consistent with the new implementation.tests/integration/tokenomics/relay_mining_difficulty_test.go (1)
195-195: LGTM!The change from
sha256.New()toprotocol.NewTrieHasher()is correct and consistent with the new implementation.
Co-authored-by: Redouane Lakrache <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testutil/proof/fixture_generators.go (2 hunks)
Additional comments not posted (3)
testutil/proof/fixture_generators.go (3)
56-58: LGTM!The changes align with the new requirements and the function looks correct.
64-73: LGTM!The changes align with the new requirements and the function looks correct.
75-88: LGTM!The function looks correct and the implementation is straightforward.
| func SmstRootWithSum(sum uint64) smt.MerkleSumRoot { | ||
| root := [protocol.TrieRootSize]byte{} | ||
| return encodeSum(root, sum) |
There was a problem hiding this comment.
Address TODO comments.
The TODO comments indicate potential tech debt and mainnet considerations.
Do you want me to help address these TODO comments or open a GitHub issue to track them?
| // encodeSum returns a copy of the given root, binary encodes the given sum, | ||
| // and stores the encoded sum in the root copy. | ||
| func encodeSum(r [protocol.TrieRootSize]byte, sum uint64) smt.MerkleSumRoot { | ||
| root := make([]byte, protocol.TrieRootSize) | ||
| copy(root, r[:]) | ||
|
|
||
| // Insert the sum into the root hash | ||
| binary.BigEndian.PutUint64(root[protocol.TrieHasherSize:], sum) | ||
| // Insert the count into the root hash | ||
| // TODO_TECHDEBT: This is a hard-coded count of 1, but could be a parameter. | ||
| // TODO_TECHDEBT: We are assuming the sum takes up 8 bytes. | ||
| binary.BigEndian.PutUint64(root[smt.SmtRootSizeBytes+8:], 1) | ||
| return smt.MerkleRoot(root[:]) | ||
| binary.BigEndian.PutUint64(root[protocol.TrieHasherSize+8:], 1) | ||
|
|
||
| return root |
There was a problem hiding this comment.
Address TODO comments.
The TODO comments indicate potential tech debt.
Do you want me to help address these TODO comments or open a GitHub issue to track them?
Co-authored-by: Redouane Lakrache <[email protected]>
…get-hash * pokt/main: [TODO] chore: update `smt.MerkleRoot#Sum()` error handling (#672)
Summary
Update
smt.MerkleRoot#Sum()error handling post-refactoring (see: pokt-network/smt#50).Issue
Outstanding
smtgo module.smtversion in go.mod.Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
Refactor
Dependencies
github.com/pokt-network/smtmodule to versionv0.12.0.Tests