feat(platform): transfer to frozen account is allowed #2478
Conversation
WalkthroughThis pull request updates the token transfer validation logic. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Validator as TokenTransferValidation
participant Sender as SenderAccount
participant Receiver as RecipientAccount
Client->>Validator: initiate state transfer validation
Validator->>Sender: fetch sender identity token info & balance
Sender-->>Validator: return sender status (frozen/not)
alt sender account frozen
Validator->>Client: return error (sender frozen)
else
Validator->>Receiver: fetch recipient token account info
Receiver-->>Validator: return recipient status (frozen/not)
alt recipient account frozen
Validator->>Client: return error (recipient frozen)
else
Validator->>Client: validation passed, proceed with transfer
end
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
shumkov
changed the title
![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
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/state_v0/mod.rs (1)
79-130: Consider refactoring frozen account checks to reduce duplication.The frozen account validation logic is duplicated for sender and recipient. Consider extracting this into a helper method to improve maintainability.
impl TokenTransferTransitionActionStateValidationV0 for TokenTransferTransitionAction { + fn validate_frozen_account( + &self, + platform: &PlatformStateRef, + identity_id: Identifier, + block_info: &BlockInfo, + execution_context: &mut StateTransitionExecutionContext, + transaction: TransactionArg, + platform_version: &PlatformVersion, + ) -> Result<SimpleConsensusValidationResult, Error> { + let (info, fee_result) = platform.drive.fetch_identity_token_info_with_costs( + self.token_id().to_buffer(), + identity_id.to_buffer(), + block_info, + true, + transaction, + platform_version, + )?; + + execution_context.add_operation(ValidationOperation::PrecalculatedOperation(fee_result)); + + if let Some(info) = info { + if info.frozen() { + return Ok(SimpleConsensusValidationResult::new_with_error( + ConsensusError::StateError(StateError::IdentityTokenAccountFrozenError( + IdentityTokenAccountFrozenError::new( + self.token_id(), + identity_id, + "transfer".to_string(), + ), + )), + )); + } + }; + + Ok(SimpleConsensusValidationResult::new()) + } + fn validate_state_v0( &self, platform: &PlatformStateRef, owner_id: Identifier, block_info: &BlockInfo, execution_context: &mut StateTransitionExecutionContext, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<SimpleConsensusValidationResult, Error> { // ... existing balance validation ... - // We need to verify that our token account is not frozen - let (info, fee_result) = platform.drive.fetch_identity_token_info_with_costs( - self.token_id().to_buffer(), - owner_id.to_buffer(), - block_info, - true, - transaction, - platform_version, - )?; - - execution_context.add_operation(ValidationOperation::PrecalculatedOperation(fee_result)); - - if let Some(info) = info { - if info.frozen() { - return Ok(SimpleConsensusValidationResult::new_with_error( - ConsensusError::StateError(StateError::IdentityTokenAccountFrozenError( - IdentityTokenAccountFrozenError::new( - self.token_id(), - owner_id, - "transfer".to_string(), - ), - )), - )); - } - }; + // Verify that sender's token account is not frozen + let validation_result = self.validate_frozen_account( + platform, + owner_id, + block_info, + execution_context, + transaction, + platform_version, + )?; + if !validation_result.is_valid() { + return Ok(validation_result); + } - // We need to verify that account we are transferring to not frozen - let (info, fee_result) = platform.drive.fetch_identity_token_info_with_costs( - self.token_id().to_buffer(), - self.recipient_id().to_buffer(), - block_info, - true, - transaction, - platform_version, - )?; - - execution_context.add_operation(ValidationOperation::PrecalculatedOperation(fee_result)); - - if let Some(info) = info { - if info.frozen() { - return Ok(SimpleConsensusValidationResult::new_with_error( - ConsensusError::StateError(StateError::IdentityTokenAccountFrozenError( - IdentityTokenAccountFrozenError::new( - self.token_id(), - self.recipient_id(), - "transfer".to_string(), - ), - )), - )); - } - }; + // Verify that recipient's token account is not frozen + let validation_result = self.validate_frozen_account( + platform, + self.recipient_id(), + block_info, + execution_context, + transaction, + platform_version, + )?; + if !validation_result.is_valid() { + return Ok(validation_result); + } // ... existing token status validation ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/state_v0/mod.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (5)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/mod.rs (1)
71-71: LGTM! Using the correct validation context.The change from
token_issuance_transition_state_validationtotoken_transfer_transition_state_validationensures that the correct validation context is used for token transfers.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/state_v0/mod.rs (4)
62-63: LGTM! Added cost tracking for balance retrieval.Properly tracking the cost of balance retrieval operation in the execution context.
89-90: LGTM! Added cost tracking for sender's token info.Properly tracking the cost of fetching sender's token info in the execution context.
105-130: LGTM! Added validation for recipient's frozen account.The implementation prevents transfers to frozen accounts by validating the recipient's token account status. The error handling is consistent with the sender's frozen account check.
This change directly addresses the PR objective of preventing transfers to frozen accounts.
139-140: LGTM! Added cost tracking for token status.Properly tracking the cost of fetching token status in the execution context.
|
The code LGTM except for breaking the test, but I'm not sure we want to do this. I can imagine scenarios where sending to a frozen identity would be useful. Like if we have a contract that collects funds in a frozen account to be unfrozen at a specific date/time. |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🔭 Outside diff range comments (2)
packages/rs-dpp/src/errors/protocol_error.rs (1)
21-21:⚠️ Potential issueRemove unused import.
The pipeline detected an unused import:
crate::state_transition::errors::StateTransitionErrorat line 21.-use crate::state_transition::errors::StateTransitionError;🧰 Tools
🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[warning] 21-21: unused import:
crate::state_transition::errors::StateTransitionError
warning: unused import:crate::state_transition::errors::StateTransitionError
--> packages/rs-dpp/src/errors/protocol_error.rs:21:5
|
21 | use crate::state_transition::errors::StateTransitionError;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Actions: Tests
[warning] 21-21: unused import:
crate::state_transition::errors::StateTransitionErrorpackages/rs-dapi-client/src/transport/wasm_channel.rs (1)
100-120: 🛠️ Refactor suggestionPotential for side effects.
into_sendspawns a local task that can lead to concurrency pitfalls if the future or channel usage is not carefully managed. Check for data races or dropped channels in real-world usage.
🧹 Nitpick comments (67)
packages/js-dapi-client/package.json (1)
85-86: Include "dist" in the published package.By adding
"dist"to the"files"array, you ensure that the built assets in thedistdirectory will be included when the package is published. Please verify that only the intended production files are present indistto avoid unintentionally publishing extra or unbuilt files.packages/js-dapi-client/lib/methods/platform/response/Metadata.js (1)
10-12: Proper handling of large integers using BigInt.Converting input values to
bigintusingBigInt()ensures precise handling of large numbers without loss of precision that could occur with JavaScript'snumbertype. This is particularly important for blockchain applications where exact integer precision is critical.Consider adding error handling around the
BigInt()conversion to gracefully handle invalid inputs:- this.height = BigInt(properties.height); + try { + this.height = BigInt(properties.height); + } catch (error) { + throw new Error(`Invalid height value: ${properties.height}`); + }packages/js-dapi-client/lib/methods/platform/getTotalCreditsInPlatform/GetTotalCreditsInPlatformResponse.js (1)
29-29: Explicit BigInt conversion implementationGood implementation of the type change by explicitly converting the value to a
bigintusingBigInt(). However, consider adding error handling for cases where the input cannot be converted to abigint.- const totalCreditsInPlatform = BigInt(proto.getV0().getCredits()); + try { + const totalCreditsInPlatform = BigInt(proto.getV0().getCredits()); + } catch (error) { + throw new InvalidResponseError('Invalid credits value: cannot be converted to BigInt'); + }packages/platform-test-suite/test/functional/platform/getStatus.spec.js (1)
20-22: Update assertion style for consistency across version checks.The assertions for checking version information use inconsistent styles:
- Line 20 uses
.to.exist()- Lines 21-22 use
.and.not.be.empty()While functionally similar, it would be better to use a consistent style across all three assertions.
-expect(status.getVersionStatus().getDapiVersion()).to.be.a('string').to.exist(); +expect(status.getVersionStatus().getDapiVersion()).to.be.a('string').and.not.be.empty();packages/rs-dpp/src/data_contract/v1/serialization/mod.rs (1)
1-1: Remove unused import.The pipeline failure indicates that the import
crate::data_contract::config::v0::DataContractConfigGettersV0is unused. Consider removing it to keep the codebase clean.-use crate::data_contract::config::v0::DataContractConfigGettersV0;🧰 Tools
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
[warning] 1-1: unused import:
crate::data_contract::config::v0::DataContractConfigGettersV0
warning: unused import:crate::data_contract::config::v0::DataContractConfigGettersV0
--> packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:1:5
|
1 | use crate::data_contract::config::v0::DataContractConfigGettersV0;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 1-1: unused import:
crate::data_contract::config::v0::DataContractConfigGettersV0
warning: unused import:crate::data_contract::config::v0::DataContractConfigGettersV0
--> packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:1:5
|
1 | use crate::data_contract::config::v0::DataContractConfigGettersV0;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 1-1: unused import:
crate::data_contract::config::v0::DataContractConfigGettersV0
warning: unused import:crate::data_contract::config::v0::DataContractConfigGettersV0
--> packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:1:5
|
1 | use crate::data_contract::config::v0::DataContractConfigGettersV0;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[warning] 1-1: unused import:
crate::data_contract::config::v0::DataContractConfigGettersV0
warning: unused import:crate::data_contract::config::v0::DataContractConfigGettersV0
--> packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:1:5
|
1 | use crate::data_contract::config::v0::DataContractConfigGettersV0;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Actions: Tests
[warning] 1-1: unused import:
crate::data_contract::config::v0::DataContractConfigGettersV0packages/js-dapi-client/lib/BlockHeadersProvider/BlockHeadersProvider.js (2)
108-109: Fix JSDoc type definitionsThe JSDoc for
initializeChainWithmethod parameters has improved parameter descriptions, but there are still issues identified by static analysis:
- The type 'BlockHeader' is undefined
- Missing JSDoc @param "firstHeaderHeight" type
- * @param {BlockHeader[]} headers array of headers - * @param firstHeaderHeight {number} first block header height + * @param {Object[]} headers array of block headers + * @param {number} firstHeaderHeight first block header height🧰 Tools
🪛 GitHub Check: JS packages (@dashevo/dapi-client) / Linting
[warning] 108-108:
The type 'BlockHeader' is undefined
[warning] 109-109:
Missing JSDoc @param "firstHeaderHeight" type
138-139: Fix parameter documentation formattingThe JSDoc comments for
readHistoricalandheadersHandlermethods have been improved, but there's still an issue with therejectparameter type inheadersHandler.- * @param {function} reject callback function + * @param {Function} reject callback functionAlso applies to: 200-201
packages/js-dapi-client/lib/test/fixtures/getStatusFixture.js (1)
1-3: Complete the JSDoc commentThe JSDoc comment at the beginning of the file is empty. Add a description of what the
getStatusFixturefunction does, including its purpose and return value./** - * + * Returns a mock status fixture object for testing purposes + * @returns {Object} A mock status object containing version, node, chain, network, stateSync and time information */packages/rs-dpp/src/bls/mod.rs (1)
1-1: Module visibility configuration simplifiedThe conditional compilation attribute for the
native_blsmodule has been simplified to only check for the "bls-signatures" feature, removing the architecture check. This change makes the module available for wasm32 targets as well.Consider adding a comment explaining why the architecture restriction was removed. This would help future developers understand the reasoning behind this change, especially if there were specific considerations for wasm32 targets.
packages/rs-dpp/src/data_contract/methods/equal_ignoring_time_based_fields/v0/mod.rs (1)
1-30: Well-documented utility method for comparing contractsThis new method provides a clean way to compare DataContract instances while ignoring time-based fields, which is useful for testing and verification scenarios.
This is a well-implemented utility method with clear documentation. Since this function ignores time-based fields specifically, consider whether there might be other use cases where you'd want to compare contracts while ignoring different sets of fields. If so, a more general comparison function that takes a configuration parameter might be more flexible for future needs.
packages/js-dapi-client/lib/methods/platform/getStatus/StateSyncStatus.js (2)
60-65: Fix incorrect JSDoc comment for getSnapshotHeight().The JSDoc comment incorrectly states this returns "Chunk process average time" when it should describe snapshot height.
/** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot height */ getSnapshotHeight() { return this.snapshotHeight; }
67-72: Fix incorrect JSDoc comment for getSnapshotChunkCount().The JSDoc comment incorrectly states this returns "Chunk process average time" when it should describe snapshot chunk count.
/** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot chunks count */ getSnapshotChunkCount() { return this.snapshotChunksCount; }packages/rs-dapi-client/src/dapi_client.rs (1)
81-83: Ensure CA certificate is properly documented.Adding a new
ca_certificatefield to theDapiClientstruct is a good enhancement for secure connections, but its relation to fixing frozen account transfers isn't clear.packages/rs-dpp/src/block/finalized_epoch_info/getters.rs (1)
7-79: Consider reducing repetitive pattern matching.All the getters follow a similar pattern of matching on
FinalizedEpochInfo::V0(v0)and forwarding calls. You could use a macro or define a single helper function that takes a closure to reduce boilerplate. However, if discoverability outweighs succinctness, leaving them as-is might be more readable.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/v0/mod.rs (1)
37-37: Use final specialized validation for success flow.Returning
Ok(SimpleConsensusValidationResult::default())for the success path is clear. Depending on future complexities, consider a specialized success flow object if you need to convey more nuanced outcomes or metadata.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (1)
17-874: Consider extracting common validation logic into helper functions.Notably, each distribution function variant performs repetitive checks such as boundary validations (e.g., zero checks, min ≤ max, etc.). Refactoring these repetitive parameter checks into reusable helpers could reduce code duplication and improve maintainability.
packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.spec.ts (2)
54-55: Added TypeScript ignore directive for BigInt additionThe TypeScript ignore directive is used to bypass type checking for BigInt addition. Consider updating the types or using proper BigInt arithmetic instead of suppressing the error.
- // @ts-ignore - .to.equal(prevNonce + BigInt(1)); + .to.equal(prevNonce + BigInt(1n));
99-100: Same TypeScript ignore directive used for BigInt additionSimilar to the previous occurrence, consider addressing the type issue properly rather than suppressing it.
- // @ts-ignore - .to.equal(prevNonce + BigInt(1)); + .to.equal(prevNonce + BigInt(1n));packages/rs-dpp/src/errors/consensus/basic/data_contract/mod.rs (1)
31-34: Consider adding doc comments for newly introduced modules.Defining separate modules for these error types follows the existing pattern, but it may be helpful to include doc comments explaining when each error is raised. This can improve clarity for future maintainers.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (4)
1-5: Remove or clarify the "adjust the import path as needed" comment.This appears to be a placeholder. If the path is now finalized, consider removing or updating the comment for clarity.
20-44: Consider using a stronger PRNG for Random variant if security is critical.The current SplitMix64-based approach is pseudorandom and deterministic for the same seed. If a cryptographically secure random distribution is needed, switch to a well-known secure PRNG.
45-83: Be mindful of floating-point inaccuracies in StepDecreasingAmount.Repeated floating operations can produce small rounding errors, which might accumulate or cause unexpected results. If precise arithmetic is crucial, consider rational or integer-based math.
432-1317: Impressive and thorough test coverage.These unit tests validate each distribution type under various edge cases. Consider adding property-based or fuzz tests if broader coverage is desired.
packages/rs-dapi-client/src/transport/tonic_channel.rs (1)
16-45: Handle channel creation errors gracefully instead of panicking.Current
.expect(...)calls at lines 21 and 42 will panic if no host is present or TLS config fails. Consider returning aTransportErroror a custom error variant instead of crashing.let host = uri.host() - .expect("Failed to get host from URI") + .ok_or_else(|| TransportError::InvalidUri("No host found".into()))? .to_string(); builder = builder - .tls_config(tls_config) - .expect("Failed to set TLS config"); + .tls_config(tls_config) + .map_err(|_| TransportError::TlsConfigError("Failed to set TLS config".into()))?;packages/rs-dpp/src/errors/consensus/basic/token/invalid_token_note_too_big_error.rs (1)
1-55: Consider ensuring the integer type can handle all potential note sizes.Currently,
u32is used to track maximum note length and actual note length. If your application might handle note sizes larger than 4 GB, consider using a broader type such asu64orusizeto prevent potential overflow on extremely large note lengths. Otherwise, the implementation here is straightforward and appears correct for typical use cases.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs (1)
192-196: Use a wildcard match arm to simplify.These arms all return
None. You could refactor this to a single_ => Nonematch arm to streamline your code, though enumerating each variant is also acceptable for explicitness.- DocumentTransition::Replace(_) => None, - DocumentTransition::Delete(_) => None, - DocumentTransition::Transfer(_) => None, - DocumentTransition::UpdatePrice(_) => None, - DocumentTransition::Purchase(_) => None, + _ => None,packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.ts (1)
49-51: Potential precision loss converting BigInt to Number.
Number(dataContractHistoryEntry.getDate().toString())might truncate if the date is beyond the safe integer range. If large timestamps are expected, consider storing them as BigInt or strings to avoid overflow.- contractHistory[Number(dataContractHistoryEntry.getDate().toString())] = ... + // Keep the date as a BigInt or string key to avoid large integer overflow. + contractHistory[dataContractHistoryEntry.getDate().toString()] = ...packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (2)
80-81: Consider a newtype for enhanced type safety
While this alias is convenient, a newtype can prevent accidental mixing of weights with otheru64values.- pub type TokenDistributionWeight = u64; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + pub struct TokenDistributionWeight(pub u64);
82-97: Remove commented-out code
Carrying large commented-out blocks may reduce maintainability. Remove them or provide explicit rationale if needed later.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/accessors.rs (1)
1-3: Remove unused import
TokenAmountis flagged as unused. Consider removing it to keep the code clean and free of unnecessary imports.- use crate::balances::credits::TokenAmount;🧰 Tools
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
[warning] 1-1: unused import:
crate::balances::credits::TokenAmount
warning: unused import:crate::balances::credits::TokenAmount
--> packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/accessors.rs:1:5
|
1 | use crate::balances::credits::TokenAmount;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 1-1: unused import:
crate::balances::credits::TokenAmount
warning: unused import:crate::balances::credits::TokenAmount
--> packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/accessors.rs:1:5
|
1 | use crate::balances::credits::TokenAmount;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 1-1: unused import:
crate::balances::credits::TokenAmount
warning: unused import:crate::balances::credits::TokenAmount
--> packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/accessors.rs:1:5
|
1 | use crate::balances::credits::TokenAmount;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[warning] 1-1: unused import:
crate::balances::credits::TokenAmount
warning: unused import:crate::balances::credits::TokenAmount
--> packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/accessors.rs:1:5
|
1 | use crate::balances::credits::TokenAmount;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Actions: Tests
[warning] 1-1: unused import:
crate::balances::credits::TokenAmountpackages/rs-dpp/src/errors/consensus/basic/basic_error.rs (1)
80-81: Use multi-line formatting
Consider splitting these error variants onto separate lines for better readability.- DestinationIdentityForTokenMintingNotSetError, InvalidActionIdError, InvalidTokenAmountError, - InvalidTokenConfigUpdateNoChangeError, InvalidTokenIdError, InvalidTokenNoteTooBigError, + DestinationIdentityForTokenMintingNotSetError, + InvalidActionIdError, + InvalidTokenAmountError, + InvalidTokenConfigUpdateNoChangeError, + InvalidTokenIdError, + InvalidTokenNoteTooBigError,packages/js-dapi-client/lib/methods/platform/getDataContractHistory/GetDataContractHistoryResponse.js (2)
3-3: Consider referencing a type definition.You might consider referencing a
.d.tsor JSDoc type definition forDataContractHistoryEntryto ensure accurate autocomplete and typings, especially if consumers rely on type information.
7-7: JSDoc improvement.Including a more descriptive explanation of how
DataContractHistoryEntry[]is structured (e.g., which fields are expected) will help maintainers/readers.packages/rs-dpp/src/errors/consensus/basic/data_contract/invalid_token_distribution_function_invalid_parameter_error.rs (2)
1-5: Explicit prefix for references.For clarity, consider prefixing top-level crates (like
platform_serialization_derive) withcrate::or fully qualifying them to help keep references consistent.
10-23: Centralize error message generation.The
#[error(...)]attribute is straightforward but can sometimes obscure error formatting if it grows complex. Consider centralizing or splitting out repeated logic if more parameters are introduced in the future.packages/rs-dapi-client/src/transport/wasm_channel.rs (4)
1-2: High-level module overview.The module documentation is concise. Consider expanding on practical usage scenarios for DAPI gRPC calls under WASM environments for new contributors unfamiliar with the approach.
6-18: Clear import grouping.You have a good set of imports from various crates. Keeping them logically grouped (e.g.,
stdimports, then crate-level imports, then external crates) may help readability if the list grows.
32-44: Constructor error handling.
WasmClient::new()returnsTransportErroron initialization failure. Ensure any potential WASM connectivity edge cases—like invalid URLs—are handled or documented thoroughly.
74-82: Enhance error context.
wasm_client_error_to_statusreturnsStatus::internal. If granular error codes or messages are relevant, consider differentiating errors (e.g., connection failures vs. timeouts) to aid debugging.packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.ts (2)
40-72:getIdentityNonce: well-structured retrieval and caching logic.
Interval-based refetch is clear. Potential improvement: handle negative or unexpectedidentityNoncevalues if your upstream logic ever returns them.
157-163: Remove unnecessary// @ts-ignorecomment if possible.
The addition ofBigInt(1)typically shouldn’t trigger type-check issues if both operands arebigint. Verify if this is still needed or can be safely removed.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs (1)
86-154: MultiplePartialEqimplementations are thorough but repetitive.
They handle comparisons against&u64,u64,&u32,u32,&u16,u16,&usize, andusize. This is correct though fairly verbose. Consider generics if duplication becomes cumbersome.packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (2)
40-58: Struct-like enums carefully define distribution states.
TokenDistributionInfoneatly encapsulates time-based vs. perpetual distribution data. Make sure you have test coverage for both variants.
86-92: Display implementation is clear and concise.No issues here. If more variants are introduced down the line, remember to extend this match.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (16)
16-20: Random variant ensures min and max are encoded.Recommend validating
min <= maxin a prior step or constructor to avoid invalid states.
41-56: Linear variant uses multiple parameters (a, d, s, b).Encoders look consistent. Confirm that the field types match decode expectations.
79-100: Exponential variant with a, d, m, n, o, s, c.Check for consistent domain constraints (e.g., negative values in exponent base?).
123-144: InvertedLogarithmic variant.Like the Logarithmic variant, confirm domain constraints are respected.
161-164: Decode for Random checks min, max.Similar to encode, confirm
min <= maxis validated to ensure correctness.
165-179: Decode for StepDecreasingAmount handles numerator/denominator.Please ensure to guard against division by zero if these fields are used in subsequent distribution logic.
202-221: Decode Polynomial with i64 + u64 interplay.Confirm proper domain ranges are tested to avoid negative exponent or coefficient.
246-265: Decode Logarithmic with i64 + u64 fields.Again, watch out for domain constraints.
268-287: Decode InvertedLogarithmic with i64 + u64 fields.Ensure negative ‘a’ or zero denominators are disallowed if needed.
307-310: BorrowDecode Random.Same domain consideration for
min <= max.
311-325: BorrowDecode StepDecreasingAmount.Similar zero denominator check recommended.
332-345: BorrowDecode Linear mixing i64 and u64.Confirm or document usage if negative slopes or offsets are valid.
348-367: BorrowDecode Polynomial.Same domain validations as the standard decode.
370-389: BorrowDecode Exponential.Check negative base or exponent.
392-411: BorrowDecode Logarithmic.Same domain constraints.
414-433: BorrowDecode InvertedLogarithmic.Ensure domain constraints are validated similarly.
packages/rs-dpp/src/data_contract/accessors/mod.rs (2)
270-277: created_at getter for V1 data contracts.Implementation is clear. Ensure tests exist for retrieving creation time.
389-394: set_created_at_epoch.No concerns; tested approach recommended.
packages/rs-dpp/src/block/finalized_epoch_info/v0/getters.rs (1)
6-43: Consider returning slices for the block proposers.Currently,
fn block_proposers(&self) -> &Vec<(Identifier, u64)>returns a reference to a concreteVec. A more flexible approach might be returning a slice&[(Identifier, u64)], which offers similar functionality but avoids exposing the underlying collection type.-fn block_proposers(&self) -> &Vec<(Identifier, u64)> { +fn block_proposers(&self) -> &[(Identifier, u64)] { &self.block_proposers }packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (4)
32-74: Clarify randomness source for “Random” variant.
The doc comments do not specify what PRF or cryptographic random method is used. If security is a requirement, ensure a cryptographically secure RNG is used under the hood.Consider adding a note or a parameter to indicate the intended random generation mechanism (e.g., chain randomness, VRF, secure RNG, etc.).
108-120: Check default/fallback behavior in “Stepwise” intervals.
If the highest key in theBTreeMapis exceeded, clarify whether the emission remains at the last defined value or proceeds with no emission. It might be beneficial to add a doc paragraph on the fallback logic for intervals not explicitly covered.
122-240: Verify linear function clamping logic.
Large slope values or extended periods risk integer overflow before clamping is applied. Confirm the arithmetic for(a * (x - s) / d)does not overflow prior to bounding withmin_valueandmax_value.I can help prepare a test or check to ensure that partial computations remain within bounds.
327-384: Double-check exponential growth calculations for potential overflow.
Whenm > 0, the exponential may rapidly grow, risking arithmetic overflow if done in integer space. For largex, consider safe integer operations or big integer libraries.
🛑 Comments failed to post (12)
packages/dapi-grpc/src/lib.rs (1)
7-45: 💡 Verification agent
🧩 Analysis chain
Verify WebAssembly module files exist and are correctly implemented.
The changes assume the existence of WebAssembly-specific module files. It's important to ensure these files exist and contain appropriate WebAssembly-compatible implementations.
🏁 Script executed:
#!/bin/bash # Check if the WebAssembly module files exist echo "Checking for core WebAssembly module file:" test -f src/core/wasm/org.dash.platform.dapi.v0.rs && echo "✅ Core WebAssembly module exists" || echo "❌ Core WebAssembly module missing" echo "Checking for platform WebAssembly module file:" test -f src/platform/wasm/org.dash.platform.dapi.v0.rs && echo "✅ Platform WebAssembly module exists" || echo "❌ Platform WebAssembly module missing"Length of output: 469
Attention: Missing WebAssembly Module Files
Our verification confirms that both the core and platform WebAssembly module files are missing. This is critical since the code conditionally includes these files for the
wasm32target. Please address the following:
Add the missing files:
src/core/wasm/org.dash.platform.dapi.v0.rssrc/platform/wasm/org.dash.platform.dapi.v0.rsAlternatively, adjust the conditional includes if the WebAssembly implementation is not required, to prevent build failures.
packages/platform-test-suite/test/functional/platform/getStatus.spec.js (1)
17-23: 💡 Verification agent
🧩 Analysis chain
Verify test coverage for frozen account transfers.
While this test appropriately checks platform status, it doesn't verify the main PR objective of preventing transfers to frozen accounts. The PR title suggests this is a security fix, but this particular test doesn't validate that functionality.
🏁 Script executed:
#!/bin/bash # Search for test cases that verify transfers to frozen accounts are prevented echo "Searching for tests related to frozen account transfers..." rg -A 5 -B 5 "frozen.*(account|identity)" --glob="*.{js,spec.js}" packages/platform-test-suite/ echo "Searching for token transfer validation tests..." rg -A 3 -B 3 "(validate.*transfer|transfer.*validate)" --glob="*.{js,spec.js}" packages/platform-test-suite/Length of output: 442
Action Required: Add tests for transfer prevention on frozen accounts
The current test in
packages/platform-test-suite/test/functional/platform/getStatus.spec.jsonly validates the platform status response and doesn’t cover the security objective of preventing transfers from frozen accounts. Our search in the test suite did not locate any test cases verifying that transfers are blocked for frozen accounts. Please add a dedicated test—or extend the existing suite—to simulate a transfer attempt from a frozen account and assert that it fails (e.g., by checking for an error or rejection).packages/rs-dapi-client/Cargo.toml (1)
8-8: 💡 Verification agent
🧩 Analysis chain
Verify impact of removing "mocks" from default features
Removing "mocks" from default features while keeping only "offline-testing" may affect existing code that relies on mock functionality. Ensure this change doesn't break any tests or dependent libraries that expect mock functionality to be available by default.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that might be affected by the removal of "mocks" from default features grep -r --include="*.rs" "mock" --include="*.toml" packages/rs-dapi-client/Length of output: 8417
Attention: Impact of Removing "mocks" from Default Features
Our search confirms that the
"mocks"feature is referenced throughout the codebase—for example, in multiple conditional compilations insrc/address_list.rs,src/dapi_client.rs,src/dump.rs,src/executor.rs,src/lib.rs,src/mock.rs,src/transport.rs, and even in tests such as intests/mock_dapi_client.rs. Removing"mocks"from the default features (leaving only"offline-testing") will disable these sections unless the feature is explicitly enabled.Action items:
- Ensure that any tests or dependent libraries relying on mock functionality are updated to explicitly enable the
mocksfeature (e.g., viacargo test --features mocks) if needed.- Update CI/test configurations and documentation to clearly communicate this breaking change.
- Verify that no critical functionality is unintentionally disabled when the mocks feature is not activated by default.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (2)
131-198: 🛠️ Refactor suggestion
Polynomial variant may truncate large intermediary results.
Similar to the linear function, casting intermediate expressions to i64 could silently overflow. Perform a bounds check before casting to i64 to ensure safe behavior.
93-130: 🛠️ Refactor suggestion
Prevent potential overflow when converting to i64 in Linear variant.
The cast
(value as i64)can silently truncate large results, bypassing the subsequentchecked_add()safety check. Before casting, consider verifying that the intermediate value is within the i64 range:let intermediate = ((*a as i128) * diff / (*d as i128)); -if intermediate < i64::MIN as i128 || intermediate > i64::MAX as i128 { - return Err(ProtocolError::Overflow("Linear function evaluation overflow or negative")); -} -let value = (intermediate as i64) +if intermediate < (i64::MIN as i128) || intermediate > (i64::MAX as i128) { + return Err(ProtocolError::Overflow( + "Linear function intermediate out of i64 bounds", + )); +} +let value = intermediate as i64 .checked_add(*b as i64) .ok_or(ProtocolError::Overflow( "Linear function evaluation overflow or negative", ))?;📝 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.// f(x) = (a * (x - s) / d) + b DistributionFunction::Linear { a, d, s, b, min_value, max_value, } => { if *d == 0 { return Err(ProtocolError::DivideByZero( "Linear function: divisor d is 0", )); } // Check that the value at x = 0 is within bounds. let s_val = s.unwrap_or(0); let diff = x.saturating_sub(s_val) as i128; let intermediate = ((*a as i128) * diff / (*d as i128)); if intermediate < (i64::MIN as i128) || intermediate > (i64::MAX as i128) { return Err(ProtocolError::Overflow( "Linear function intermediate out of i64 bounds", )); } let value = (intermediate as i64) .checked_add(*b as i64) .ok_or(ProtocolError::Overflow( "Linear function evaluation overflow or negative", ))?; let value = if value < 0 { 0 } else { value as u64 }; if let Some(min_value) = min_value { if value < *min_value { return Ok(*min_value); } } if let Some(max_value) = max_value { if value > *max_value { return Ok(*max_value); } } Ok(value as TokenAmount) }packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs (2)
111-113: 🛠️ Refactor suggestion
Verify placeholder logic for TokenClaimTransition.
This method always returns
None, making the newly added transition inaccessible. If this is intentional (e.g., a work in progress), consider adding a TODO comment or implementing it fully. Otherwise, calls expecting a token claim transition will fail.
119-121: 🛠️ Refactor suggestion
Verify placeholder logic for TokenConfigUpdateTransition.
Similar to
as_transition_token_claim, this method returnsNone. Confirm that no functionality requiring aTokenConfigUpdateTransitionreference is currently expected. If so, consider marking this as a TODO or implementing the logic to avoid confusion later.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (1)
62-68:
⚠️ Potential issueUse checked addition for the iteration variable.
While you already use
checked_addfor accumulatingtotal, you addsteptoxdirectly, which can overflow ifx + stepexceedsu64::MAX. For consistency, handle this with checked arithmetic:- x += step; + x = x.checked_add(step).ok_or_else(|| { + ProtocolError::Overflow("Block height overflow in evaluate_interval".into()) + })?;📝 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.while x <= end_included { // Call evaluate(x) and accumulate the result. total = total.checked_add(self.evaluate(x)?).ok_or_else(|| { ProtocolError::Overflow("Total evaluation overflow in evaluate_interval".into()) })?; x = x.checked_add(step).ok_or_else(|| { ProtocolError::Overflow("Block height overflow in evaluate_interval".into()) })?; }packages/rs-dpp/src/errors/consensus/codes.rs (1)
105-108: 🛠️ Refactor suggestion
Duplicate error codes.
Both
InvalidTokenDistributionFunctionDivideByZeroErrorandInvalidTokenDistributionFunctionInvalidParameterErrorshare the same code10254. This may lead to ambiguity or confusion when handling errors. Consider assigning unique codes to each variant for more precise error identification.- Self::InvalidTokenDistributionFunctionDivideByZeroError(_) => 10254, - Self::InvalidTokenDistributionFunctionInvalidParameterError(_) => 10254, + Self::InvalidTokenDistributionFunctionDivideByZeroError(_) => 10254, + Self::InvalidTokenDistributionFunctionInvalidParameterError(_) => 10255,Committable suggestion skipped: line range outside the PR's diff.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (1)
101-122: 🛠️ Refactor suggestion
Logarithmic variant additions.
No specific issues, but ensure you handle domain restrictions for logs (no zero or negative arguments).
Potentially add validations to avoid invalid log computations.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (1)
454-512: 🛠️ Refactor suggestion
Prevent domain errors in “InvertedLogarithmic” variant.
As with the regular Logarithmic variant, ensure(m * (x - s + o))never causes a zero or negative denominator insiden / (m * …).Add explicit checks or doc disclaimers on domain constraints to avoid runtime panics.
packages/rs-dpp/src/data_contract/accessors/v1/mod.rs (1)
96-97:
⚠️ Potential issueFix doc mismatch for
fn set_updated_at_epoch().
The function doc incorrectly states “Sets the block height at which the contract was last updated.” but the method sets the epoch, not block height.-/// Sets the block height at which the contract was last updated. +/// Sets the epoch at which the contract was last updated. fn set_updated_at_epoch(&mut self, epoch: Option<EpochIndex>);📝 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./// Sets the epoch at which the contract was last updated. fn set_updated_at_epoch(&mut self, epoch: Option<EpochIndex>);
QuantumExplorer
changed the title
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
4 participants
Issue being fixed or feature implemented
Transfer to frozen account is currently allowed.
What was done?
How Has This Been Tested?
With existing tests
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit