feat!: update grpc supply query#7137
Conversation
Updated the grpc GetTokensInCirculation query to provide: - mined_rewards at height - spendable_rewards at height - spendable_pre_mine at height - total_spendable at height
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPC Server
participant ConsensusManager
Client->>gRPC Server: Request ValueAtHeight(height)
gRPC Server->>ConsensusManager: block_rewards_mined_at_height(height)
gRPC Server->>ConsensusManager: block_rewards_spendable_at_height(height)
gRPC Server->>ConsensusManager: pre_mine_spendable_at_height(height)
gRPC Server->>ConsensusManager: total_tokens_spendable_at_height(height)
ConsensusManager-->>gRPC Server: Returns values for each query
gRPC Server-->>Client: ValueAtHeightResponse (height, mined_rewards, spendable_rewards, spendable_pre_mine, total_spendable)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
base_layer/core/src/blocks/pre_mine/mod.rs (1)
69-69: Nit: refine doc comment wording
Consider changing "average amount of blocks per day" to "average number of blocks per day" for grammatical clarity.applications/minotari_app_grpc/proto/base_node.proto (1)
229-233: Enhance API docs for new fields
Consider adding protobuf comments above each new field to describe their semantics (e.g., unit, calculation method) and improve generated documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_app_grpc/proto/base_node.proto(1 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(2 hunks)base_layer/core/src/blocks/pre_mine/mod.rs(1 hunks)base_layer/core/src/consensus/consensus_manager.rs(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/core/src/consensus/consensus_manager.rs (4)
base_layer/core/src/blocks/pre_mine/mod.rs (1)
pre_mine_spendable_at_height(172-179)base_layer/core/src/consensus/chain_strength_comparer.rs (1)
strongest_chain(137-139)base_layer/core/tests/helpers/test_blockchain.rs (1)
consensus_manager(84-86)base_layer/core/src/transactions/tari_amount.rs (2)
from_str(187-211)from_str(353-361)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (10)
base_layer/core/src/blocks/pre_mine/mod.rs (1)
69-70: Make BLOCKS_PER_DAY public for cross-module use
ExposingBLOCKS_PER_DAYaligns with its consumption in consensus calculations and gRPC layers. The added doc comment improves discoverability.applications/minotari_app_grpc/proto/base_node.proto (1)
229-233: Update ValueAtHeightResponse to granular token-supply breakdown
The new fields (mined_rewards,spendable_rewards,spendable_pre_mine,total_spendable) replace the old aggregated supply values. This aligns the API with updated consensus methods. As this is a breaking change, ensure all downstream clients regenerate their stubs and adjust to the new response structure.applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)
62-62: LGTM: Good cleanup of unused import.Removing the unused
emission::Emissionimport improves code cleanliness and reduces unnecessary dependencies.
367-377: LGTM: Well-implemented breaking change for granular token supply reporting.The updated response construction properly utilizes the new consensus manager methods to provide detailed token supply information:
mined_rewards: Total rewards mined at heightspendable_rewards: Spendable block rewards at heightspendable_pre_mine: Spendable pre-mine tokens at heighttotal_spendable: Total spendable tokens at heightThe implementation correctly handles errors with the
?operator and uses appropriate type conversions. This breaking change aligns with the PR objectives to provide more accurate and complete token supply information.base_layer/core/src/consensus/consensus_manager.rs (6)
29-29: LGTM: Import addition supports new functionality.The import for
pre_mine_spendable_at_heightis appropriately added to support the new methods.
39-45: LGTM: Import reorganization improves code organization.Moving the base_node-specific imports under the feature flag is good practice for conditional compilation.
190-194: LGTM: Method correctly calculates total spendable tokens.The implementation properly sums spendable block rewards and spendable pre-mine at the given height. Error propagation is handled correctly using the
?operator.
196-199: LGTM: Simple and correct wrapper method.The method appropriately delegates to the imported function with proper error handling.
201-204: LGTM: Method correctly calculates mined block rewards.The logic of subtracting pre-mine value from total emission to get mined rewards is correct and uses the appropriate consensus constants for the given height.
357-438: LGTM: Comprehensive test validates the new functionality.The test thoroughly validates all four calculation methods across various heights including edge cases. The test data includes realistic values and covers different time periods (180 days, 365 days, etc.). The use of
BLOCKS_PER_DAYconstant makes the test more readable and maintainable.
Test Results (CI) 3 files 126 suites 40m 6s ⏱️ Results for commit 61b8d9e. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files 10 suites 2h 42m 36s ⏱️ For more details on these failures, see this check. Results for commit 61b8d9e. ♻️ This comment has been updated with latest results. |
v4.3.1-pre.0
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
base_layer/core/src/consensus/consensus_manager.rs (2)
202-205: Sub-underflow guard
self.get_total_emission_at(height) - self.consensus_constants(height).pre_mine_value()will panic if, for
some future network,supply_at_block(0)is <pre_mine_value().
Safer to usesaturating_sub(already used elsewhere in the file).- Ok(self.get_total_emission_at(height) - self.consensus_constants(height).pre_mine_value()) + Ok(self + .get_total_emission_at(height) + .saturating_sub(self.consensus_constants(height).pre_mine_value()))
360-438: Flaky parsing risk in test vectors
MicroMinotari::from_str(" 13946753.809464 T")relies on the parser tolerating leading spaces.
If that implementation ever tightens, the test will panic at compile time.
Strip padding to make the literals future-proof.- MicroMinotari::from_str(" 13946753.809464 T"), // mined + MicroMinotari::from_str("13946753.809464 T"),(Apply to all literals with leading spaces.)
base_layer/core/src/transactions/transaction_components/transaction_output.rs (3)
52-57: Import grouping nitThe
#[cfg(feature = "base_node")]block that follows could include theExtendedRangeProofService
import to keep conditional dependencies co-located, improving readability.
242-271:verify_range_proofalloc churn
vec![&self.proof_result()?.0]allocates twice (Vec + underlyingRangeProofVec).
Since the batch size is always 1 here, consider bypassingverify_batchand calling the single-proof
API directly to avoid needless heap work.
Not critical, just a micro-optimisation.
564-607: Batch verification error aggregation
batch_verify_range_proofsexits on the first error.
For diagnostics it can be useful to return which output(s) failed.
If the underlying API can give that information, bubble it up in a custom error so callers can log the
offending commitments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_app_grpc/proto/base_node.proto(1 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(2 hunks)base_layer/core/src/common/waiting_requests.rs(2 hunks)base_layer/core/src/consensus/consensus_manager.rs(4 hunks)base_layer/core/src/transactions/transaction_components/transaction_output.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/core/src/common/waiting_requests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- applications/minotari_node/src/grpc/base_node_grpc_server.rs
- applications/minotari_app_grpc/proto/base_node.proto
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (1)
base_layer/core/src/consensus/consensus_manager.rs (1)
236-241: 👍 Consistent dynamic pre-mine usageSwapping the hard-coded constant for
self.consensus_constants(height).pre_mine_value()fixes the inconsistency previously flagged.
No further action needed.
| extended_range_proof::ExtendedRangeProofService, | ||
| tari_utilities::hex::Hex, | ||
| }; | ||
| #[cfg(feature = "base_node")] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused import when base_node feature is disabled
ExtendedRangeProofService is only referenced inside #[cfg(feature = "base_node")]
functions. Building without that feature triggers an unused import warning (which CI treats as an error in this repo).
-use tari_crypto::{
- commitment::HomomorphicCommitmentFactory,
- errors::RangeProofError,
- extended_range_proof::ExtendedRangeProofService,
- tari_utilities::hex::Hex,
-};
+use tari_crypto::{
+ commitment::HomomorphicCommitmentFactory,
+ errors::RangeProofError,
+ #[cfg(feature = "base_node")]
+ extended_range_proof::ExtendedRangeProofService,
+ tari_utilities::hex::Hex,
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extended_range_proof::ExtendedRangeProofService, | |
| tari_utilities::hex::Hex, | |
| }; | |
| #[cfg(feature = "base_node")] | |
| use tari_crypto::{ | |
| commitment::HomomorphicCommitmentFactory, | |
| errors::RangeProofError, | |
| #[cfg(feature = "base_node")] | |
| extended_range_proof::ExtendedRangeProofService, | |
| tari_utilities::hex::Hex, | |
| }; |
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_components/transaction_output.rs
around lines 49 to 52, the import of ExtendedRangeProofService is unused when
the base_node feature is disabled, causing a warning treated as an error. To fix
this, conditionally compile the import by adding #[cfg(feature = "base_node")]
above the import statement so it is only included when the base_node feature is
enabled.
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
2 participants
Description
Updated the grpc GetTokensInCirculation query to provide:
Motivation and Context
The grpc query did not provide the correct information.
How Has This Been Tested?
Added unit test
fn test_supply_at_block()System-level testing tot test the grpc query.
What process can a PR reviewer use to test or verify this change?
Code review
Breaking Changes
BREAKING CHANGE: The grpc method
GetTokensInCirculationnow use an updated return struct asSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests