feat: CoinJoin #592
feat: CoinJoin #592
Conversation
feat(coinjoin): first steps for DashSync <-> DashSharedCore integration
feat(coinjoin): WalletEx callbacks
feat(coinjoin): availableCoins method
feat(coinjoin): selectCoinsGroupedByAddresses
feat(coinjon): create_denominated callbacks
feat(coinjoin): commit transaction
feat(coinjoin): refactor no prompt signing and availableCoins
fix(coinjoin): wait for denom tx to be published before making a collateral
feat(coinjoin) ClientManager callbacks
chore(coinjoin): refactor some code into CoinJoinManager
feat(coinjoin): send message callbacks
feat(coinjoin): recalculate balance
# Conflicts: # DashSync/shared/Models/Identity/DSBlockchainIdentity.h # DashSync/shared/Models/Identity/DSBlockchainIdentity.m # DashSync/shared/Models/Identity/DSBlockchainInvitation.m # DashSync/shared/Models/Identity/DSInvitation.h
feat: merge dashpay / coinjoin
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
♻️ Duplicate comments (4)
DashSync/shared/Models/CoinJoin/DSCompactTallyItem.m (1)
32-41:⚠️ Potential issueImplement robust memory allocation error handling
The
ffi_malloc:method allocates memory forvaluesbut doesn't check if the allocation succeeds. If memory allocation fails, accessing this null pointer could lead to crashes.Implement proper error checking after memory allocation:
- NSUInteger count = self.inputCoins.count; - DInputCoin **values = malloc(count * sizeof(DInputCoin *)); - for (NSUInteger i = 0; i < count; i++) { - values[i] = [self.inputCoins[i] ffi_malloc:type]; - } - DInputCoins *input_coins = DInputCoinsCtor(count, values); - return DCompactTallyItemCtor(bytes_ctor(self.txDestination), self.amount, input_coins); + NSUInteger count = self.inputCoins.count; + DInputCoin **values = malloc(count * sizeof(DInputCoin *)); + if (values == NULL) { + return NULL; // Handle memory allocation failure + } + + BOOL success = YES; + for (NSUInteger i = 0; i < count; i++) { + values[i] = [self.inputCoins[i] ffi_malloc:type]; + if (values[i] == NULL) { + success = NO; + break; + } + } + + if (!success) { + // Free already allocated memory + for (NSUInteger i = 0; i < count; i++) { + if (values[i] != NULL) { + [DSCompactTallyItem ffi_free:values[i]]; + } + } + free(values); + return NULL; + } + + DInputCoins *input_coins = DInputCoinsCtor(count, values); + return DCompactTallyItemCtor(bytes_ctor(self.txDestination), self.amount, input_coins);DashSync/shared/Models/CoinJoin/DSTransactionInput+CoinJoin.m (1)
44-52:⚠️ Potential issueAdd memory allocation validation for robustness.
The memory allocated using
mallocis not checked for allocation failures. If the allocation fails, the code will crash when attempting to use the allocated memory.Implement a check after memory allocation:
+ (DTxInputs *)ffi_to_tx_inputs:(NSArray<DSTransactionInput *> *)obj { NSUInteger count = obj.count; DTxIn **values = malloc(count * sizeof(DTxIn *)); + if (!values && count > 0) { + NSLog(@"Failed to allocate memory for DTxIn array"); + return NULL; + } for (NSUInteger i = 0; i < count; i++) { values[i] = [obj[i] ffi_malloc]; + if (!values[i]) { + NSLog(@"Failed to allocate memory for DTxIn at index %lu", (unsigned long)i); + // Clean up previously allocated inputs + for (NSUInteger j = 0; j < i; j++) { + DTxInDtor(values[j]); + } + free(values); + return NULL; + } } return DTxInputsCtor(count, values); }DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.m (1)
44-48:⚠️ Potential issueAdd out-of-bounds check on output index.
When
inputTxis found, there is no check to ensureindexis within the valid range ofinputTx.outputs. Ifindexis invalid, accessinginputTx.outputs[index].outScriptcould cause a runtime exception.Consider applying the following diff:
if (inputTx) { + if (index >= inputTx.outputs.count) { + // Handle invalid index, e.g., log or skip + continue; + } script = inputTx.outputs[index].outScript; }DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (1)
984-991:⚠️ Potential issuePossible division by zero in printUsedKeys.
This block reimplements a known concern from a past review:double percent = (double)usedAddresses.count * 100.0 / (double)issuedAddresses.count;Guard against
issuedAddresses.countbeing zero to avoid a potential crash. This repeats a previously flagged comment.Apply a check before division:
- double percent = (double)usedAddresses.count * 100.0 / (double)issuedAddresses.count; + double percent = 0.0; + if (issuedAddresses.count > 0) { + percent = (double)usedAddresses.count * 100.0 / (double)issuedAddresses.count; + }
🧹 Nitpick comments (116)
DashSync/shared/Models/Chain/DSMerkleBlock.m (1)
118-146: Avoid redundant assignment ofmerkleRoot.Currently,
self.merkleRootis assigned both in-[DSMerkleBlock initWithBlockHash:merkleRoot:totalTransactions:hashes:flags:]and again here at line 136. This second assignment is redundant and can be removed to reduce confusion:- self.merkleRoot = merkleRoot;DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Platform.m (2)
24-247: Use meaningful error codes and domains.All errors currently return an
NSErrorwith a code of0and do not specify an error domain. This makes it hard to distinguish different types of errors in the client code. Consider defining a unique error code for each failure path and providing a suitable domain (e.g.,org.dash.core.error) to improve error handling and debugging.
150-150: Correct the typo in the error message.“Corruped Code Execution” contains a spelling error. Use “Corrupted Code Execution” to ensure clarity and professionalism in error messages.
DashSync/shared/Models/Chain/DSChainLock.m (1)
42-42: Consider thread safety for the newly added pointer property.
Usingnonatomic, assignfor theDChainLock *lockpointer is acceptable if you’re certain there is no concurrent access from multiple threads. Otherwise, consider thread-safe patterns (e.g., atomic properties or synchronization).DashSync/shared/Models/Derivation Paths/DSDerivationPath.m (3)
91-118: Serialized extended public key creation.
Deserializes viaDSDerivationPathFactory; the approach is solid. Potentially consider explicit error handling ifdeserializedExtendedPublicKeyis invalid.
536-538: Optional private key storage.
Storing a private key in the keychain is reasonable, but consider encrypting it or leveraging secure enclave features to reinforce security.
622-625: Safely retrieving extended private key data.
Consider clearer error handling or logging if retrieval fails.DashSync/shared/Models/Entities/DSAssetUnlockTransactionEntity+CoreDataClass.m (1)
48-48: Explicit version assignment for special transactionsThe added line sets
tx.version = SPECIAL_TX_VERSIONwhich ensures that asset unlock transactions have the proper version for special transaction types. This is important for compatibility with the Dash protocol specifications.However, make sure that
SPECIAL_TX_VERSIONis properly defined in a header file that is included in this implementation file.Consider adding a comment explaining why this specific version is required for asset unlock transactions.
DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Dash.m (1)
38-44: Useful utility method for error consolidationThis new method provides a convenient way to combine multiple error descriptions, which is particularly valuable for complex operations like CoinJoin where multiple errors might need to be reported.
The implementation correctly handles the concatenation of error messages with newline separators.
Consider handling the case when the errors array is empty or nil:
+ (NSString *)errorsDescription:(NSArray<NSError *> *)errors { + if (errors.count == 0) { + return @""; + } NSMutableString *description = [NSMutableString string]; for (NSError *error in errors) { [description appendFormat:@"%@\n", error.localizedDescription]; } return description; }DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataProperties.h (1)
26-26:Details
❓ Verification inconclusive
Remove commented out code rather than commenting it
The
masternodeproperty has been commented out rather than properly removed. Commented-out code adds noise to the codebase and can cause confusion about whether it's intended to be restored later.-//@property (nullable, nonatomic, retain) DSSimplifiedMasternodeEntry *masternode;Additionally, since this is a Core Data entity property, removing a relationship will require updating the Core Data model. Ensure that the corresponding changes have been made to the model.
🏁 Script executed:
#!/bin/bash # Check if this change is reflected in the Core Data model grep -r "DSGovernanceVoteEntity.*masternode" --include="*.xcdatamodeld*"Length of output: 74
Action: Remove the Commented-Out
masternodeProperty and Confirm Core Data Model UpdatesPlease remove the commented-out
masternodeproperty fromDashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataProperties.hto keep the codebase clean. Also, since this property belongs to a Core Data entity, verify manually (in your.xcdatamodeldfiles) that the corresponding relationship has been removed from the Core Data model. The initial grep check did not return any results, so please double-check that no residual references tomasternoderemain.DashSync/shared/Models/Entities/DSBlockchainIdentityKeyPathEntity+CoreDataProperties.m (1)
23-24: New Core Data properties need documentationTwo new Core Data properties have been added:
securityLevelandpurpose. These properties align with the identity management enhancements related to CoinJoin functionality.Consider adding:
- Documentation comments explaining the expected values and usage for these properties
- Validation to ensure these properties are properly initialized in relevant creation methods
- Default values if appropriate
These properties likely relate to key management requirements for CoinJoin, but their specific meaning and valid values should be documented.
DashSync/shared/Models/Entities/DSContractEntity+CoreDataClass.m (1)
14-18: Add method documentation and consider return value clarityThe new convenience method for retrieving contract entities is well-implemented but lacks documentation.
Consider adding documentation to clarify:
- The method's purpose and usage scenarios
- Whether it returns nil if no matching entity exists
- Whether this method is meant only for retrieval or if it should create a new entity when none exists
+/** + * Retrieves a contract entity with the specified identifier on the given chain. + * @param identifier The local contract identifier to search for + * @param chain The chain context for the contract + * @param context The managed object context + * @return The matching contract entity or nil if none exists + */ + (instancetype)entityWithLocalContractIdentifier:(NSString *)identifier onChain:(DSChain *)chain inContext:(NSManagedObjectContext *)context { return [DSContractEntity anyObjectInContext:context matching:@"localContractIdentifier == %@ && chain == %@", identifier, [chain chainEntityInContext:context]]; }DashSync/shared/Categories/NSDate+Utils.m (1)
15-20: Utility methods for time calculations look goodThese utility methods provide convenient ways to calculate timestamps in the past, which is helpful for various timeout and expiration scenarios in CoinJoin operations.
Consider adding documentation comments to clarify their purpose:
+/** + * Returns the current time interval since 1970 minus the specified interval. + * @param interval The time interval to subtract from the current time + * @return NSTimeInterval representing current time minus the specified interval + */ + (NSTimeInterval)timeIntervalSince1970Minus:(NSTimeInterval)interval { return [NSDate timeIntervalSince1970] - interval; } +/** + * Returns the current time interval since 1970 minus one hour (3600 seconds). + * @return NSTimeInterval representing current time minus one hour + */ + (NSTimeInterval)timeIntervalSince1970MinusHour { return [NSDate timeIntervalSince1970] - 3600; }DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataProperties.h (1)
28-29: Added relationship to blockchain identityThe addition of the
identityproperty creates a relationship between asset lock transactions and blockchain identities, which aligns with the CoinJoin functionality being implemented. This property is correctly marked as nullable and properly defined with appropriate memory management attributes.Consider documenting the purpose of this relationship with a comment to clarify how it's used in the CoinJoin process.
DashSync/shared/Categories/NSObject+Notification.h (2)
1-3: Copyright year is set to 2025The copyright year is set to 2025, which is in the future (current year is 2024).
-// Copyright © 2025 Dash Core Group. All rights reserved. +// Copyright © 2024 Dash Core Group. All rights reserved.
22-26: Added notification utility method to NSObjectThe addition of the
notify:userInfo:method provides a convenient shorthand for posting notifications. This is a good abstraction that will help reduce boilerplate code throughout the application.Consider adding a brief comment explaining the method's purpose and how it should be used.
DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Dash.h (2)
22-23: Consider a more flexible error macro designThe
ERROR_500macro is hardcoded with error code 500, which limits its reusability. Consider creating a more general macro that allows specifying different error codes, similar to the commented-outDS_ERRORmacro.-#define ERROR_500(msg) [NSError errorWithCode:500 localizedDescriptionKey:msg] +#define ERROR_WITH_CODE(code, msg) [NSError errorWithCode:code localizedDescriptionKey:msg]Then you could use it as
ERROR_WITH_CODE(500, msg)for your specific case while maintaining flexibility.
32-32: Missing documentation for new methodPlease add a documentation comment for the new
errorsDescription:method that explains its purpose, parameters, and return value. This will help other developers understand how to use it correctly.+/** + * Returns a formatted string description of an array of errors. + * @param errors An array of NSError objects to be described. + * @return A concatenated string of error descriptions. + */ + (NSString *)errorsDescription:(NSArray<NSError *> *)errors;DashSync/shared/Categories/NSSet+Dash.h (1)
30-35: Ensure proper memory management with FFI methodsThe FFI interface for
Vec_u8_32includes methods for creation and destruction, which is good. However, ensure that callers offfi_from_vec_u256understand they're responsible for callingffi_destroy_vec_u256to prevent memory leaks.Consider adding documentation comments that clarify this responsibility:
+/** + * Creates an NSSet<NSData *> from a Rust Vec_u8_32. + * Note: This does not destroy the original Vec_u8_32. Caller must call ffi_destroy_vec_u256 when done. + * @param ffi_ref Pointer to the Rust Vec_u8_32. + * @return An autoreleased NSSet containing NSData objects. + */ + (NSSet<NSData *> *)ffi_from_vec_u256:(Vec_u8_32 *)ffi_ref; +/** + * Converts an NSSet<NSData *> to a Rust Vec_u8_32. + * Note: Caller is responsible for destroying the returned Vec_u8_32 using ffi_destroy_vec_u256. + * @param obj The NSSet to convert. + * @return A newly allocated Vec_u8_32 that must be destroyed with ffi_destroy_vec_u256. + */ + (Vec_u8_32 *)ffi_to_vec_u256:(NSSet<NSData *> *)obj; +/** + * Destroys a Rust Vec_u8_32 object and frees its memory. + * @param ffi_ref Pointer to the Rust Vec_u8_32 to destroy. + */ + (void)ffi_destroy_vec_u256:(Vec_u8_32 *)ffi_ref;DashSync/shared/Models/Entities/DSBlockchainIdentityKeyPathEntity+CoreDataProperties.h (1)
24-25: Consider using strongly typed enumerations for new properties.
DefiningsecurityLevelandpurposeasuint8_tis concise but may reduce readability or lead to confusion down the line. It can be beneficial to switch to typed enums to make the code more self-documenting and to prevent invalid assignments.-@property (nonatomic, assign) uint8_t securityLevel; -@property (nonatomic, assign) uint8_t purpose; +typedef NS_ENUM(uint8_t, DSIdentitySecurityLevel) { + DSIdentitySecurityLevelUndefined = 0, + // ... +}; +typedef NS_ENUM(uint8_t, DSIdentityPurpose) { + DSIdentityPurposeUndefined = 0, + // ... +}; +@property (nonatomic, assign) DSIdentitySecurityLevel securityLevel; +@property (nonatomic, assign) DSIdentityPurpose purpose;DashSync/shared/Models/CoinJoin/DSInputCoin.h (1)
27-37: Clarify the Objective-C naming for FFI methods and ensure memory management is clear.
The properties foroutpointHash,outpointIndex, andeffectiveValueseem correct. However, methods such as- (DInputCoin *)ffi_malloc:(DChainType *)typeand+ (void)ffi_free:are somewhat unusual in Objective-C. Consider adding clarifying documentation or an Objective-C friendly naming scheme. Also confirm that ownership (who is responsible for freeing the memory) is clearly documented to avoid leaks or confusion.DashSync/shared/Categories/NSIndexPath+FFI.h (1)
23-28: Ensure consistent memory management documentation.
These new class methods for converting betweenNSIndexPathandVec_u32pointers (and destroying them) are beneficial. However, adding a brief note explaining the expected ownership and lifecycle of the returned pointers will help avoid memory leaks and confusion about the correct usage offfi_destroy:.DashSync/shared/Models/Entities/DSDashpayUserEntity+CoreDataClass.h (1)
48-49: Consider clarifying the error domain and codes.
The method- (NSError *)applyTransientDashpayUser:save:potentially returnsNSError. It's good practice to define the error domain and codes so consumers of this API can handle failures reliably. Documenting the expected failure scenarios would further streamline usage.DashSync/shared/Categories/NSData/NSData+DSMerkAVLTree.m (1)
18-19: Import changes reflect architectural shift in platform handlingThese commented-out imports suggest a move away from the platform-specific query and CBOR decoding functionality. This aligns with the broader architectural changes mentioned in the PR summary.
Rather than commenting out imports, consider removing them entirely if they're no longer needed, as commented code can lead to confusion about whether this is temporary or permanent.
DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataClass.m (1)
23-28: Import additions enhance integration but verify usage.The new imports suggest expanded functionality and dependencies. While appropriate for the changes made, ensure these dependencies are actually needed in this implementation.
DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataClass.h (1)
26-28: New method for loading local masternodes.This class method enhances the API by providing a way to load all local masternodes for a specific chain. Consider adding documentation to explain the purpose and usage of this method.
-+ (void)loadLocalMasternodesInContext:(NSManagedObjectContext *)context - onChainEntity:(DSChainEntity *)chainEntity; ++ (void)loadLocalMasternodesInContext:(NSManagedObjectContext *)context + onChainEntity:(DSChainEntity *)chainEntity; +/** + * Loads all local masternodes associated with the specified chain entity in the given context. + * @param context The managed object context to perform the operation in + * @param chainEntity The chain entity to filter masternodes by + */DashSync/shared/Models/CoinJoin/DSCoinControl.m (3)
22-39: Good initialization from FFI object.The initializer properly handles the conversion from the FFI coin control object to the Objective-C implementation. Consider adding braces around the if-statement body for consistency and to prevent future bugs.
- if (coinControl->dest_change) - self.destChange = [DSKeyManager NSStringFrom:DAddressWithScriptPubKeyData(coinControl->dest_change, chainType)]; + if (coinControl->dest_change) { + self.destChange = [DSKeyManager NSStringFrom:DAddressWithScriptPubKeyData(coinControl->dest_change, chainType)]; + }
36-36: Complex line converting DOutPoint to DSUTXO.This line combines multiple operations including a cast and multiple function calls. Consider adding a comment or extracting to a helper method for clarity.
- [setSelected addObject:dsutxo_obj(((DSUTXO){u256_cast(dashcore_hash_types_Txid_inner(outpoint->txid)), outpoint->vout}))]; + // Convert DOutPoint to DSUTXO and add to selected set + UInt256 txid = u256_cast(dashcore_hash_types_Txid_inner(outpoint->txid)); + DSUTXO utxo = {txid, outpoint->vout}; + [setSelected addObject:dsutxo_obj(utxo)];
41-56: Good default initialization.The basic initializer properly sets default values for all properties. However, consider using
nilinstead ofNULLfor the_destChangeproperty since this is Objective-C.- _destChange = NULL; + _destChange = nil;DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.h (2)
23-33: Add documentation to clarify property units and meanings.The balance properties are defined without documentation explaining what each property represents. This makes it harder for developers to understand what each balance type means or what units they're in (satoshis?).
@interface DSCoinJoinBalance : NSObject +/// Balance in regular (non-denominated) inputs that is confirmed and trusted, in satoshis @property (nonatomic, assign) uint64_t myTrusted; +/// Balance in denominated inputs that is confirmed and trusted, in satoshis @property (nonatomic, assign) uint64_t denominatedTrusted; +/// Balance that has gone through the CoinJoin process and is anonymized, in satoshis @property (nonatomic, assign) uint64_t anonymized; +/// Balance that is still immature (recently mined), in satoshis @property (nonatomic, assign) uint64_t myImmature; +/// Balance in untrusted pending transactions, in satoshis @property (nonatomic, assign) uint64_t myUntrustedPending; +/// Balance in denominated untrusted pending transactions, in satoshis @property (nonatomic, assign) uint64_t denominatedUntrustedPending; +/// Watch-only trusted balance, in satoshis @property (nonatomic, assign) uint64_t watchOnlyTrusted; +/// Watch-only untrusted pending balance, in satoshis @property (nonatomic, assign) uint64_t watchOnlyUntrustedPending; +/// Watch-only immature balance, in satoshis @property (nonatomic, assign) uint64_t watchOnlyImmature;
46-50: Document FFI methods for better API understanding.The FFI (Foreign Function Interface) methods lack documentation explaining their purpose, requirements, and ownership semantics. This information is crucial for developers who need to use these methods correctly.
@interface DSCoinJoinBalance (FFI) +/// Converts a DSCoinJoinBalance object to a DBalance pointer. +/// @param obj The DSCoinJoinBalance object to convert. +/// @return A newly allocated DBalance pointer that must be destroyed with ffi_destroy:. + (DBalance *)ffi_to:(DSCoinJoinBalance *)obj; +/// Destroys a DBalance pointer previously created with ffi_to:. +/// @param ffi_ref The DBalance pointer to destroy. + (void)ffi_destroy:(DBalance *)ffi_ref;DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Platform.h (3)
1-16: Copyright year needs correction.The copyright year is set to 2025, which is in the future. It should be updated to the current year (2024).
-// Copyright © 2025 Dash Core Group. All rights reserved. +// Copyright © 2024 Dash Core Group. All rights reserved.
24-26: Document error conversion methods.The error conversion method lacks documentation explaining what error domain and code it uses, and any special handling for different error types. This makes it harder for developers to understand how to handle these errors.
@interface NSError (dash_spv_platform_error_Error) +/// Converts a platform error from the Rust FFI to an NSError with the appropriate domain and code. +/// @param ffi_ref The platform error reference from the Rust FFI. +/// @return An NSError object with the appropriate domain, code, and userInfo derived from the platform error. + (NSError *)ffi_from_platform_error:(dash_spv_platform_error_Error *)ffi_ref; @end
29-43: Apply consistent documentation to all error categories.Similar to the previous comment, all error conversion methods in the various categories should be documented to provide clear guidance on error domains, codes, and special handling.
The pattern shown in the previous comment should be applied to all conversion methods in this file:
- Add a description of what the method does
- Document parameters
- Document return values
- Include any special handling or considerations
DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.h (2)
13-13: Document the large CoinJoin gap limit.The new constant
SEQUENCE_GAP_LIMIT_INITIAL_COINJOINis set to 400, which is significantly larger than other gap limits. This deserves explanation through a comment to help developers understand why this large value is necessary.+// CoinJoin requires a much larger gap limit to accommodate the numerous addresses used in the mixing process #define SEQUENCE_GAP_LIMIT_INITIAL_COINJOIN 400
57-57: Add documentation for the CoinJoin derivation path method.The new method
coinJoinDerivationPathForAccountNumber:onChain:lacks documentation explaining its purpose, how it differs from other derivation path methods, and when it should be used.+/** + * Creates a derivation path specifically for CoinJoin operations for the given account number on the specified chain. + * This path uses a larger gap limit (SEQUENCE_GAP_LIMIT_INITIAL_COINJOIN) to accommodate the numerous addresses + * used during the CoinJoin mixing process. + * + * @param accountNumber The account number to use for this derivation path + * @param chain The blockchain to use + * @return A new DSFundsDerivationPath instance configured for CoinJoin operations + */ + (instancetype)coinJoinDerivationPathForAccountNumber:(uint32_t)accountNumber onChain:(DSChain *)chain;DashSync/shared/DSDashSharedCore.h (3)
24-32: Consider wrapping these macro definitions with a more descriptive namespace or static constants.
Using extensive defines for type aliases can obscure intent and may lead to conflicts. Opting for typed constants or a dedicated header can improve maintainability and reduce potential macro collisions.
38-38: Assess whether a designated initializer is needed.
Although- (instancetype)initOnChain:is helpful, you might consider making it the designated initializer or explicitly marking- (init)as unavailable to avoid situations where the object is instantiated without a valid chain.
50-50: Rename boolean property to follow Objective-C naming conventions.
For properties of type BOOL, consider a name likeisMasternodeListCurrentlyBeingSavedto make usage clearer.- @property (nonatomic, readonly) BOOL hasMasternodeListCurrentlyBeingSaved; + @property (nonatomic, readonly) BOOL isMasternodeListCurrentlyBeingSaved;DashSync/shared/Libraries/Logs/CompressingLogFileManager.m (2)
155-172: Consider making retention duration configurable.
Hardcoding a 5-day cutoff for old logs may not suit all environments. Allowing it to be set via configuration or class property can make the log management more flexible.
459-473: Avoid partial files if cleanup fails.
When compression fails, you remove the output file. Log or handle the case where cleanup also fails, to prevent stale or partial.gzfiles. Currently, removal failure is just logged as an error. Depending on the environment, consider more robust fallback logic.DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.m (1)
22-44: Add documentation to clarify the purpose of each balance field.
Consider adding comments for parameters to help others understand the difference betweenmyTrusted,denominatedTrusted, andanonymizedfields. This improves clarity for contributors new to the code.DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.m (1)
76-91: Validatemallocusage and potential memory leaks.Allocating arrays (
DTxIn **andDTxOut **) without error handling is typical in bridging code, but ensure that all allocated memory is freed in failure paths to avoid leaks if something goes wrong before the end of this method.DashSync/shared/Models/Derivation Paths/DSGapLimit.h (1)
36-41: Revisitreadwritescope.
directionis marked asreadwritebut might be more robust if declaredreadonlywith an initializer or constructor method setting it, preventing accidental mutation in code that consumes this class.DashSync/shared/Models/Chain/DSChainLock.h (2)
35-36: Property naming might cause confusion
The propertyDChainLock *lock;might overshadow concurrency or locking semantics. Consider using a more descriptive name such aschainLockData.-@property (nonatomic, readonly) DChainLock *lock; +@property (nonatomic, readonly) DChainLock *chainLockData;
47-52: Initializer enhancements
Consider documenting howheightis used internally, as there's no corresponding property. Providing a doc comment clarifies usage for future maintainers.DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.m (3)
86-90: Switch statement recommended
The repeated if-else fortype->tagcan be replaced with a switch statement for improved readability and maintainability.- if (type->tag == dash_spv_crypto_network_chain_type_ChainType_MainNet) { - ... - } else if (type->tag == dash_spv_crypto_network_chain_type_ChainType_TestNet) { - ... - } else if (type->tag == dash_spv_crypto_network_chain_type_ChainType_DevNet) { - ... + switch (type->tag) { + case dash_spv_crypto_network_chain_type_ChainType_MainNet: + // ... + break; + case dash_spv_crypto_network_chain_type_ChainType_TestNet: + // ... + break; + case dash_spv_crypto_network_chain_type_ChainType_DevNet: + // ... + break; + default: + NSAssert(FALSE, @"Unknown ChainType"); + break; + }
96-96: Usage ofrecoverKnownDevnetWithIdentifier
Ensure the function handles errors gracefully. Consider adding error handling or logs if recovery fails.
142-142: Debug logging
Review ifDSLog(@"Removing extra chain entity...")is intended for production or debug logs.DashSync/shared/Models/Chain/DSChain+Transaction.h (2)
28-29: Property naming
allTransactionsis intuitive for retrieving transactions in descending date order. Ensure concurrency safety if used across multiple threads.
37-38: Docstring typo
"trasaction" is misspelled.-/*! @brief Returns the amount sent globally by the trasaction ... +/*! @brief Returns the amount sent globally by the transaction ...DashSync/shared/DSDashSharedCore.m (9)
195-197: Implement the TODO or remove the placeholder.
You currently returnNULLinget_data_contract_caller(), but there's aTODOindicating unimplemented logic. Relying on aNULLreturn might cause upstream errors if any caller expects a valid contract object.Should I open a new issue or provide a proposed implementation stub for this method?
250-254: Potential concurrency risk with@synchronized(context).
In methodget_block_hash_by_height_caller(), using@synchronized(context)could be fragile ifcontextis replaced or freed unexpectedly. Synchronizing onselfor another internal lock object is often more reliable.-@synchronized (context) { +@synchronized (self) { block = (DSBlock *)[core.chain blockAtHeight:block_height]; ... }
163-168: Consider moving hardcoded IP addresses to configuration.
Embedding a long list of IP addresses in the code can hinder maintainability if updates or additions become necessary. A static configuration file or a dynamically fetched list might be more flexible.
121-183: Consider replacing hard-coded IP addresses.
Hard-coding so many addresses for mainnet may be less maintainable long-term. Using DNS seeds or dynamic bootstrapping can improve flexibility and reduce maintenance overhead.
195-200: Unimplemented callback placeholders.
The functionsget_data_contract_caller,get_platform_activation_height_caller, andcallback_can_sign_callercurrently return stubs. This could break functionality if these callbacks are triggered in production.Would you like a helper implementation or a new issue ticket to track these?
Also applies to: 221-225, 226-230
232-259: Blocking calls in callback.
blockUntilGetInsightForBlockHash:andblockUntilGetInsightForBlockHeight:may cause lengthy blocking if insight requests are slow. Consider asynchronous or background handling to avoid potential performance bottlenecks.
163-169: Consider externalizing the mainnet IP addresses.
Hardcoding this large list of IP addresses may cause maintenance overhead when peers change or become unavailable. Storing them in a configuration file, remote API, or chain params module could improve flexibility and maintainability.
195-198: Implement the data contract retrieval logic.
Currently, this method is an unimplemented placeholder. This will likely cause errors or null data returns in production.Would you like help creating a default implementation or a stub that avoids potential crashes?
232-245: Handle potential race conditions when fetching block height.
You use@synchronized(context)in other places but not here, which might lead to inconsistent chain state if multiple threads callget_block_height_by_hash_caller.DashSync/shared/DashSync.h (1)
9-9: Validate newly added imports and forward declarations.
Most additions are new category imports (e.g.,DSChain+Identity.h,DSChain+Params.h, etc.) and identity-related headers. Ensure there are no cyclical imports. Typically, using forward declarations in headers can improve build times if the implementations are only needed in .m files.Also applies to: 12-12, 14-17, 19-19, 24-24, 35-40, 67-69, 115-115
DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.m (2)
29-39: Avoid code duplication across identity funding derivation path constructors
The methodsidentityRegistrationFundingDerivationPathForWallet:,identityTopupFundingDerivationPathForWallet:, andidentityInvitationFundingDerivationPathForWallet:all have similar logic that invokesDSDerivationPathFactory. Consider consolidating them into a single factory method with parameters to reduce duplication.
92-95: Return address fallback
receiveAddressreturns either the newly registered addresses or the last known ordered address. Consider logging or handling the scenario when both are nil to avoid silent failures.DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataClass.h (2)
29-29: Remove unneeded forward references if not used
@class DSChainLockEntity;is declared but isn't evidently referenced in this header. If it's not used, consider removing it to keep the code clean.
52-53: Optimize repeated block existence checks
hasBlocksWithHash:inContext:may be called repeatedly for large sync operations. Consider indexing or more efficient lookups in Core Data to avoid performance bottlenecks.DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.m (4)
9-9: Ensure all new imports are needed
Imports forDSAddressEntity+CoreDataClass.h,DSIdentity.h,DSChain+Params.h, andDSGapLimit.hare newly added. Verify that each import is actually used to avoid unnecessary dependencies.Also applies to: 11-12, 15-15
97-97: Avoid silent errors when reading from Keychain
When readinghasKnownBalanceUniqueIDStringfrom the Keychain, consider handling errors more explicitly so a Keychain failure or data corruption doesn’t silently alterhasKnownBalanceInternal.
112-113: Remove or explain commented-out code
The commented-out line inhasKnownBalanceUniqueIDStringcould cause confusion. If it's obsolete, remove it; otherwise, clarify why it's intentionally commented out.
303-324: Review Core Data usage
Methods likeloadAddressesInContext:andstoreNewAddressesInContext:internal:context:useperformBlock:orperformBlockAndWait:. Confirm that all calls happen on the correct queue and handle concurrency/in-memory merges properly. Also consider deferring or batchingds_saveto enhance performance if numerous addresses are created in short succession.Also applies to: 326-344
DashSync/shared/Models/Chain/DSChain+Identity.m (2)
121-126: Core Data context usage
wipeIdentitiesPersistedDataInContext:(lines 121–126) performs deletion in a blocking[context performBlockAndWait:]. If large amounts of data exist, consider using[context performBlock:]or batch deletes to avoid blocking the main thread.
145-195: Remove or clarify large commented-out sections
The commented-out methods (lines 145–195) are not actively used. If you plan to revive them, consider adding aTODO:note with details. Otherwise, removing them could reduce code clutter.-//-(BOOL)registerBlockchainIdentityRegistrationTransaction: ... -// ... -//- (BOOL)registerIdentityTopupTransaction: ... -// ... +// If these methods are no longer needed, consider removing them entirely.DashSync/shared/Models/Chain/DSChain+Transaction.m (8)
50-59: Potential performance consideration
This method iterates over all wallets to find the transaction. In large multi-wallet scenarios, consider indexing or caching for faster lookups. Otherwise, the logic is clear.
61-67: Potentially large array creation
Collecting all transactions across wallets can yield a large set if many transactions exist. Ensure memory usage is acceptable in high-volume scenarios.
69-78: Typographical fix
"retuns" is misspelled in the comment. Consider changing to "returns." Otherwise, the logic is straightforward.
80-89: Typographical fix
"retuns" is misspelled in the comment. Otherwise, method naming and logic mirroramountReceivedFromTransactioncorrectly.
93-139: Expand coverage for asset lock/unlock transitions
There's a TODO hinting at additional handling for asset lock/unlock transitions. If relevant, consider completing that for consistency with the other special transactions.Would you like me to open a new issue or help implement the missing coverage for these transaction types?
200-222: Consistent multi-wallet handling
Similar logic ensures coverage for voting and operator wallets. As with provider registration, consider verifying that the transaction always references correct wallets for completeness.
233-273: Handling of topup transactions
A TODO mentions topup transaction handling. As more transaction types grow, the expanded if-else chain may become unwieldy. Consider extracting specialized methods or using a strategy pattern for better clarity.
290-345: Future expansion
The method triggers updates for multiple special transactions. There's commented-out code for DSAssetUnlockTransaction. Implementing it or removing the placeholder helps avoid confusion.DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.m (1)
79-132: Thread safety with @synchronized
@synchronized(self)ensures concurrency safety for address generation. If this section is called frequently, consider decoupling time-consuming portions to avoid potential lock contention.DashSync/shared/Models/CoinJoin/Utils/DSMasternodeGroup.m (5)
248-295: Consider refactoringgetNextPendingMasternodefor clarity and maintainability.
This method is relatively large and performs multiple steps—looping through sessions, retrieving addresses, checking backoff times, etc. Splitting it into smaller helper methods (e.g., retrieving candidate peers separately from picking the least backoff time) can reduce complexity.
494-507: Log usage inpeerConnected:can leak private details.
While the logs are helpful, including location and counts could reveal network topology if logs are exposed incorrectly. Ensure that logs are sanitized or properly secured to avoid unintentional data exposure.
39-58: Check for potential lock ordering or nested locks
This class leverages multiple@synchronized(...)locks (e.g.,peersLock,mutablePendingSessions,mutablePendingClosingMasternodes, etc.). While each lock is used for distinct data structures, be mindful of lock ordering to ensure no inadvertent nested lock scenarios. A consistent lock acquisition order across methods helps avoid deadlocks.
134-156: AvoidNSAssertin production
Whenwarn == YESand the peer is not found, the code triggersNSAssert. In production builds, this can crash the app. Prefer graceful error handling (e.g., logging an error and returning) rather than asserting.- NSAssert(NO, @"Cannot find %@", [self hostFor:ip]); + DSLog(@"[%@] CoinJoin: peer not found: %@", self.chain.name, [self hostFor:ip]); // Possibly return early or handle more gracefully
193-245: Potential strong reference toselfin dispatch block
triggerConnectionsJobWithDelay:schedules a block that capturesselfstrongly. While typically acceptable, ensure that this queue-based scheduling won’t outlive the object’s lifecycle or prevent deallocation when_isRunningis switched off.DashSync/shared/Models/Chain/DSChain.h (4)
67-84: Validate memory management for newly introduced Rust references.
Properties likesharedRuntime,sharedPlatformObj, etc., are nullable references to Rust bridging objects. Ensure that these references are properly retained and not prematurely freed (or left dangling) by the Rust side.
234-235: Clarify usage ofnewAddressesForBloomFilter.
This newly added method returns “possibly new” addresses for the bloom filter. Validate that it doesn't inadvertently reveal PII or private addresses. If these addresses are partially used or ephemeral, ensure that they’re properly tracked to avoid re-use or confusion.
304-305: Check for consistent error handling inbadMasternodeListReceivedFromPeer:.
This newly introduced delegate method indicates a serious condition if the list is corrupted. Investigate if the chain or sync process should attempt a fallback or re-request the list.Shall I provide a fallback flow or new issue to handle this scenario?
67-84: Consistent naming for shared resources
@property (nonatomic, nullable) DSDashSharedCore *shareCore;might be more consistent if namedsharedCore. Ensure naming remains intuitive (e.g., “sharedCore” or “dashSharedCore”) so it aligns with the actual class name.DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.h (1)
24-53: Consider clarifying the purpose of bridging macros.
These macros (e.g.,DPoolState,DPoolMessage) reference Rust bridging code but lack inline documentation. Adding short doc comments will help future contributors understand their usage without referencing external documentation or Rust bindings.DashSync/shared/Models/Chain/DSChain+Checkpoint.h (1)
29-30: Reinstate or revise the commented-out doc comment.
Line 29 has a commented-out doc reference for thecheckpointsproperty. Consider switching to/*! @brief … */or///style to make the documentation visible in generated docs.DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.m (2)
209-210: Avoid usingsleep(1)in production code.
Thesleep(1)call can cause unnecessary delays and is prone to race conditions in a multi-threaded environment. Prefer a more robust synchronization mechanism to ensure addresses are loaded.
309-327: Review potential performance impact ofperformBlockAndWait:in loops.
When generating many new addresses, repeatedly callingperformBlockAndWait:can block the main thread. Consider batching or using asynchronous operations to avoid UI responsiveness issues.DashSync/shared/Models/Chain/DSChain+Checkpoint.m (4)
24-29: Consider using the standard* constpattern for constant strings.The current code defines constant strings using
const *syntax:NSString const *checkpointsKey = @"checkpointsKey";The more conventional Objective-C pattern is
* const:NSString * const checkpointsKey = @"checkpointsKey";This ensures both the pointer and the string content are constant.
96-104: Consider optimizing the search for masternodes list.The current implementation iterates through all checkpoints to find ones with masternode lists. For better performance with larger checkpoint collections, consider maintaining a separate index or sorted array of checkpoints with masternode lists.
131-152: Optimize memory allocation in createCheckpointsArrayFromCheckpoints.The method creates a mutable array but returns an immutable copy:
NSMutableArray *checkpointMutableArray = [NSMutableArray array]; // ... add objects return [checkpointMutableArray copy];Consider either:
- Using
autoreleasewith the copy to prevent memory leaks- Using
NSArray arrayWithArray:instead of copy- If the array doesn't need to be immutable, return the mutable array directly
69-72: Remove unnecessary empty line in the lastTerminalCheckpoint method.The method contains an extra empty line after the return statement which is not needed.
- (DSCheckpoint *)lastTerminalCheckpoint { return self.terminalHeadersOverrideUseCheckpoint ? self.terminalHeadersOverrideUseCheckpoint : [self lastCheckpoint]; - }DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.m (2)
171-235: Consider refactoring registerAddressesWithSettings method.This method is complex (65 lines) and handles multiple responsibilities. Consider refactoring to:
- Split the identity handling into separate methods
- Extract the address generation logic
- Use early returns to reduce nesting
The current implementation has duplicate code for processing addresses at the beginning and after synchronization.
237-256: Inconsistent use of uint32_t vs NSUInteger for indices.The method
firstUnusedPublicKeyusesuint32_tfor the index, whilepublicKeyDataForHash160:casts fromNSUIntegertouint32_t:uint32_t index = [self firstUnusedIndex]; // In firstUnusedPublicKey uint32_t index = (uint32_t)[self indexOfKnownAddressHash:hash160]; // In publicKeyDataForHash160Consider standardizing on a single type for indices throughout the class to improve consistency.
DashSync/shared/Models/Chain/DSChain+Wallet.m (4)
31-31: Consider using a static char for the associated object key.
DefiningmWalletsKeyas anNSString *for the associated object key is valid but somewhat unconventional. Typically, a static char is used for clarity and to avoid potential collisions.-NSString const *mWalletsKey = @"mWalletsKey"; +static char mWalletsKey; // Then pass &mWalletsKey to objc_(get|set)AssociatedObject
61-66: Verify thread safety for 'mWallets' usage.
mWalletsis a mutable array accessed via associated objects. If multiple threads might modify it, consider adding locks or a thread-safe container to avoid data races.- (NSMutableArray<DSWallet *> *)mWallets { return objc_getAssociatedObject(self, &mWalletsKey); } - (void)setMWallets:(NSMutableArray<DSWallet *> *)mWallets { objc_setAssociatedObject(self, &mWalletsKey, mWallets, OBJC_ASSOCIATION_RETAIN_NONATOMIC); } + (NSMutableArray<DSWallet *> *)lockedMWallets { + // Example approach using a synchronized accessor + @synchronized (self) { + return objc_getAssociatedObject(self, &mWalletsKey); + } +}
139-151: Ensure keychain retrieval errors are handled.
getKeychainArraymay fail, returning nil and an error. The implementation checksif (!error && walletIdentifiers), but if there’s a valid error, it’s currently ignored. Logging or handling the fallback explicitly would make error conditions more transparent.
157-167: Reduce code repetition in wallet-having-hash lookup methods.
Identical logic is repeated when searching for identity locks, provider authentication keys, and so on. Consider refactoring into a single generic method to avoid duplication and reduce maintenance effort.Also applies to: 170-180, 183-193, 195-206, 208-219, 221-232, 234-245
DashSync/shared/Models/Chain/DSChain+Wallet.h (1)
31-34: Fix spelling in doc comments.
“Conveniance” → “Convenience” and “walleet” → “wallet” to maintain clarity and professionalism in documentation.-/*! @brief Conveniance method. Does this walleet have a chain? */ +/*! @brief Convenience method. Does this wallet have a chain? */DashSync/shared/Models/Derivation Paths/DSDerivationPathFactory.m (3)
20-24: Clarify usage of define constants.
Ensure that these path-related defines (e.g.,DERIVATION_PATH_EXTENDED_PUBLIC_KEY_WALLET_BASED_LOCATION) are used uniformly across the codebase. If they are only used here, consider static constants within this file or an enum-based approach for better type safety.
295-301: MasterIdentityContactsDerivationPath usage.
Creating a “master contact path” might need testing if it’s new or partially implemented. Verify usage across identity-related features.
482-502: String representation of index for derivation paths.
The method elegantly handles both numeric and hex-based cases, including a path to retrieve username strings. This is a nice UX improvement. Keep an eye on performance if used repeatedly in tight loops.DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (4)
149-220: startAsync, start, doMaintenance, and core logic.
Multiple scheduling calls, event notifications, and concurrency blocks. Check for possible double-notifications if the timer is canceled unexpectedly.
488-516: countInputsWithAmount logic.
Looks fine, but if you query huge amounts of UTXOs frequently, consider caching or indexing. Otherwise this is acceptable for typical usage.
518-646: availableCoins method.
This central method handles many coin selection scenarios. The code is large but well structured. Consider factoring out specialized checks to smaller helpers for clarity.
787-813: getSpendableTXs for coin selection.
Builds a set of transactions by scanning unspent outputs. This could be expensive if repeated often, but workable for smaller sets.DashSync/shared/Models/Chain/DSChain+Params.m (1)
46-63: Runtime associations for chain properties.
Storing chain data viaobjc_setAssociatedObjectis a pragmatic approach. No direct issues, but keep concurrency in mind if multi-threaded.DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (3)
58-137: Consolidate@synchronized(self)usage
These lines introduce many Rust bridging callbacks that all use@synchronized (self)or@synchronized (context). While this is convenient for preventing concurrent FFI calls, it centralizes all thread contention on a single lock. Consider partitioning locks or ensuring the locking overhead is acceptable for high-throughput scenarios.
198-210: Validate peer parameters
InprocessDSQueueFrom:andprocessMessageFrom:, the code constructs aSocketAddrfromDSPeer. Confirm that IPv4 vs. IPv6 addresses are interpreted correctly and consistently with how Rust expects them (especially for IPv4-mapped IPv6 scenarios).
536-542: Avoid concurrency overhead for simple property checks
isBlockchainSyncedcalls@synchronized (context). Ifmanager.isChainSyncedis a simple property read, consider whether you can use more granular locking or atomic properties to reduce overhead and the risk of lock contention.DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.h (1)
24-28: Add explanatory docstrings for new identity derivation methods.
These newly introduced methods reflect key identity funding steps, but lack doc comments describing usage. Clear documentation can help maintainers.+/// Returns a derivation path for registration funding of an identity. +/// @param wallet The wallet to derive from. +/// @return A derivation path configured for identity registration funding. + (instancetype)identityRegistrationFundingDerivationPathForWallet:(DSWallet *)wallet;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (194)
.gitmodules(0 hunks)DAPI-GRPC.podspec(0 hunks)DashSync.podspec(2 hunks)DashSync/iOS/Models/Managers/Service Managers/Auth/Controllers/DSBasePinViewController.m(1 hunks)DashSync/shared/Categories/BigIntTypes.h(1 hunks)DashSync/shared/Categories/NSArray+Dash.h(2 hunks)DashSync/shared/Categories/NSArray+Dash.m(1 hunks)DashSync/shared/Categories/NSData/NSData+DSHash.m(1 hunks)DashSync/shared/Categories/NSData/NSData+DSMerkAVLTree.h(1 hunks)DashSync/shared/Categories/NSData/NSData+DSMerkAVLTree.m(1 hunks)DashSync/shared/Categories/NSData/NSData+Dash.h(1 hunks)DashSync/shared/Categories/NSData/NSData+Dash.m(3 hunks)DashSync/shared/Categories/NSData/NSMutableData+Dash.m(1 hunks)DashSync/shared/Categories/NSDate+Utils.h(1 hunks)DashSync/shared/Categories/NSDate+Utils.m(1 hunks)DashSync/shared/Categories/NSIndexPath+Dash.m(1 hunks)DashSync/shared/Categories/NSIndexPath+FFI.h(1 hunks)DashSync/shared/Categories/NSIndexPath+FFI.m(1 hunks)DashSync/shared/Categories/NSMutableArray+Dash.h(1 hunks)DashSync/shared/Categories/NSObject+Notification.h(2 hunks)DashSync/shared/Categories/NSObject+Notification.m(2 hunks)DashSync/shared/Categories/NSSet+Dash.h(2 hunks)DashSync/shared/Categories/NSSet+Dash.m(1 hunks)DashSync/shared/Categories/NSString+Bitcoin.m(1 hunks)DashSync/shared/Categories/NSString+Dash.m(1 hunks)DashSync/shared/DSDashSharedCore.h(1 hunks)DashSync/shared/DSDashSharedCore.m(1 hunks)DashSync/shared/DashSync.h(4 hunks)DashSync/shared/DashSync.m(2 hunks)DashSync/shared/DashSync.xcdatamodeld/.xccurrentversion(1 hunks)DashSync/shared/DashSync.xcdatamodeld/DashSync 21.xcdatamodel/contents(1 hunks)DashSync/shared/DashSync.xcdatamodeld/DashSync 22.xcdatamodel/contents(1 hunks)DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Dash.h(1 hunks)DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Dash.m(1 hunks)DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Platform.h(1 hunks)DashSync/shared/Libraries/AdvancedOperations/Others/NSError+Platform.m(1 hunks)DashSync/shared/Libraries/DSUInt256IndexPath.h(0 hunks)DashSync/shared/Libraries/Logs/CompressingLogFileManager.m(1 hunks)DashSync/shared/Models/Chain/DSBlock.m(1 hunks)DashSync/shared/Models/Chain/DSChain+Checkpoint.h(1 hunks)DashSync/shared/Models/Chain/DSChain+Checkpoint.m(1 hunks)DashSync/shared/Models/Chain/DSChain+Identity.h(1 hunks)DashSync/shared/Models/Chain/DSChain+Identity.m(1 hunks)DashSync/shared/Models/Chain/DSChain+Params.h(1 hunks)DashSync/shared/Models/Chain/DSChain+Params.m(1 hunks)DashSync/shared/Models/Chain/DSChain+Protected.h(3 hunks)DashSync/shared/Models/Chain/DSChain+Transaction.h(1 hunks)DashSync/shared/Models/Chain/DSChain+Transaction.m(1 hunks)DashSync/shared/Models/Chain/DSChain+Wallet.h(1 hunks)DashSync/shared/Models/Chain/DSChain+Wallet.m(1 hunks)DashSync/shared/Models/Chain/DSChain.h(9 hunks)DashSync/shared/Models/Chain/DSChainConstants.h(3 hunks)DashSync/shared/Models/Chain/DSChainLock.h(1 hunks)DashSync/shared/Models/Chain/DSChainLock.m(6 hunks)DashSync/shared/Models/Chain/DSCheckpoint.h(1 hunks)DashSync/shared/Models/Chain/DSCheckpoint.m(2 hunks)DashSync/shared/Models/Chain/DSFullBlock.m(1 hunks)DashSync/shared/Models/Chain/DSMerkleBlock.h(1 hunks)DashSync/shared/Models/Chain/DSMerkleBlock.m(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinControl.h(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinControl.m(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.h(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.m(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinManager.h(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.h(1 hunks)DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m(1 hunks)DashSync/shared/Models/CoinJoin/DSCompactTallyItem.h(2 hunks)DashSync/shared/Models/CoinJoin/DSCompactTallyItem.m(1 hunks)DashSync/shared/Models/CoinJoin/DSInputCoin.h(1 hunks)DashSync/shared/Models/CoinJoin/DSInputCoin.m(1 hunks)DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.h(2 hunks)DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.m(1 hunks)DashSync/shared/Models/CoinJoin/DSTransactionInput+CoinJoin.h(2 hunks)DashSync/shared/Models/CoinJoin/DSTransactionInput+CoinJoin.m(1 hunks)DashSync/shared/Models/CoinJoin/Utils/DSMasternodeGroup.m(1 hunks)DashSync/shared/Models/DAPI/DSDAPIClient.h(0 hunks)DashSync/shared/Models/DAPI/DSDAPIClient.m(0 hunks)DashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.h(0 hunks)DashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.m(0 hunks)DashSync/shared/Models/DAPI/DSPlatformPathQuery.h(0 hunks)DashSync/shared/Models/DAPI/DSPlatformPathQuery.m(0 hunks)DashSync/shared/Models/DAPI/DSPlatformQuery.h(0 hunks)DashSync/shared/Models/DAPI/DSPlatformQuery.m(0 hunks)DashSync/shared/Models/DAPI/DSPlatformTreeQuery.h(0 hunks)DashSync/shared/Models/DAPI/DSPlatformTreeQuery.m(0 hunks)DashSync/shared/Models/DAPI/NSPredicate+CBORData.h(0 hunks)DashSync/shared/Models/DAPI/NSPredicate+CBORData.m(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIClientFetchDapObjectsOptions.h(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIClientFetchDapObjectsOptions.m(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkService.h(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkService.m(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkServiceProtocol.h(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.h(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.m(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.h(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.m(0 hunks)DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkServiceProtocol.h(0 hunks)DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath+Protected.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.m(1 hunks)DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath+Protected.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.m(5 hunks)DashSync/shared/Models/Derivation Paths/DSCreditFundingDerivationPath+Protected.h(0 hunks)DashSync/shared/Models/Derivation Paths/DSCreditFundingDerivationPath.m(0 hunks)DashSync/shared/Models/Derivation Paths/DSDerivationPath+Protected.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSDerivationPath.h(8 hunks)DashSync/shared/Models/Derivation Paths/DSDerivationPath.m(14 hunks)DashSync/shared/Models/Derivation Paths/DSDerivationPathFactory.h(2 hunks)DashSync/shared/Models/Derivation Paths/DSDerivationPathFactory.m(6 hunks)DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.h(2 hunks)DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.m(10 hunks)DashSync/shared/Models/Derivation Paths/DSGapLimit.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSGapLimit.m(1 hunks)DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.m(7 hunks)DashSync/shared/Models/Derivation Paths/DSMasternodeHoldingsDerivationPath.h(0 hunks)DashSync/shared/Models/Derivation Paths/DSMasternodeHoldingsDerivationPath.m(2 hunks)DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath+Protected.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.h(1 hunks)DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.m(8 hunks)DashSync/shared/Models/Entities/DSAccountEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSAddressEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSAddressEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSAddressEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataClass.m(3 hunks)DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataProperties.h(2 hunks)DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSAssetUnlockTransactionEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataClass.m(2 hunks)DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataProperties.h(3 hunks)DashSync/shared/Models/Entities/DSBlockchainIdentityKeyPathEntity+CoreDataProperties.h(2 hunks)DashSync/shared/Models/Entities/DSBlockchainIdentityKeyPathEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.m(8 hunks)DashSync/shared/Models/Entities/DSChainEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSChainEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataClass.m(2 hunks)DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSContactRequest.h(0 hunks)DashSync/shared/Models/Entities/DSContactRequest.m(0 hunks)DashSync/shared/Models/Entities/DSContractEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSContractEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataClass.m(0 hunks)DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSDashpayUserEntity+CoreDataClass.h(2 hunks)DashSync/shared/Models/Entities/DSDerivationPathEntity+CoreDataClass.m(4 hunks)DashSync/shared/Models/Entities/DSFriendRequestEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataProperties.h(1 hunks)DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataClass.h(1 hunks)DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataClass.m(2 hunks)DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataProperties.h(1 hunks)DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataClass.h(2 hunks)DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataProperties.m(1 hunks)DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.m(0 hunks)DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataClass.h(2 hunks)DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataClass.m(3 hunks)DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSPeerEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataClass.m(0 hunks)DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataClass.m(0 hunks)DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.h(0 hunks)DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.m(0 hunks)DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataProperties.h(0 hunks)DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataProperties.m(0 hunks)DashSync/shared/Models/Entities/DSTransactionEntity+CoreDataClass.m(1 hunks)DashSync/shared/Models/Governance/DSGovernanceObject.m(2 hunks)DashSync/shared/Models/Governance/DSGovernanceVote.h(3 hunks)DashSync/shared/Models/Governance/DSGovernanceVote.m(2 hunks)DashSync/shared/Models/Identity/DSBlockchainIdentity+Protected.h(0 hunks)DashSync/shared/Models/Identity/DSBlockchainIdentity.h(0 hunks)
💤 Files with no reviewable changes (62)
- .gitmodules
- DashSync/shared/Models/Entities/DSAddressEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataClass.h
- DashSync/shared/Models/DAPI/Networking/DSDAPIClientFetchDapObjectsOptions.h
- DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkService.h
- DashSync/shared/Libraries/DSUInt256IndexPath.h
- DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataProperties.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkServiceProtocol.h
- DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkService.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.h
- DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataClass.m
- DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/Networking/DSDAPICoreNetworkServiceProtocol.h
- DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataProperties.h
- DashSync/shared/Models/Derivation Paths/DSCreditFundingDerivationPath.m
- DashSync/shared/Models/Derivation Paths/DSCreditFundingDerivationPath+Protected.h
- DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataProperties.m
- DashSync/shared/Models/DAPI/DSPlatformQuery.m
- DashSync/shared/Models/Identity/DSBlockchainIdentity+Protected.h
- DashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.m
- DashSync/shared/Models/Entities/DSCreditFundingTransactionEntity+CoreDataClass.h
- DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataProperties.h
- DashSync/shared/Models/Entities/DSChainEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataProperties.h
- DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.m
- DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataClass.m
- DashSync/shared/Models/DAPI/DSDAPIClient.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.m
- DashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.h
- DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.m
- DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.m
- DashSync/shared/Models/Entities/DSContactRequest.m
- DashSync/shared/Models/Derivation Paths/DSMasternodeHoldingsDerivationPath.h
- DashSync/shared/Models/DAPI/NSPredicate+CBORData.h
- DashSync/shared/Models/Entities/DSQuorumCommitmentTransactionEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/DSPlatformPathQuery.m
- DashSync/shared/Models/Entities/DSAddressEntity+CoreDataProperties.h
- DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.h
- DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.h
- DashSync/shared/Models/DAPI/DSPlatformTreeQuery.m
- DAPI-GRPC.podspec
- DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataClass.m
- DashSync/shared/Models/Entities/DSQuorumEntryEntity+CoreDataClass.h
- DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataProperties.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIClientFetchDapObjectsOptions.m
- DashSync/shared/Models/DAPI/DSPlatformPathQuery.h
- DashSync/shared/Models/DAPI/DSPlatformQuery.h
- DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/DSPlatformTreeQuery.h
- DashSync/shared/Models/Entities/DSChainEntity+CoreDataProperties.h
- DashSync/shared/Models/DAPI/NSPredicate+CBORData.m
- DashSync/shared/Models/Entities/DSContactRequest.h
- DashSync/shared/Models/Entities/DSQuorumSnapshotEntity+CoreDataClass.h
- DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataProperties.m
- DashSync/shared/Models/Identity/DSBlockchainIdentity.h
- DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.h
- DashSync/shared/Models/DAPI/DSDAPIClient.h
- DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataProperties.h
✅ Files skipped from review due to trivial changes (16)
- DashSync/shared/Categories/NSMutableArray+Dash.h
- DashSync/shared/Models/Entities/DSPeerEntity+CoreDataClass.m
- DashSync/shared/Categories/NSIndexPath+Dash.m
- DashSync/shared/Models/Chain/DSBlock.m
- DashSync/shared/Categories/NSString+Bitcoin.m
- DashSync/shared/DashSync.xcdatamodeld/.xccurrentversion
- DashSync/shared/Categories/NSData/NSMutableData+Dash.m
- DashSync/shared/Categories/NSString+Dash.m
- DashSync/shared/Models/Entities/DSTransactionEntity+CoreDataClass.m
- DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataProperties.m
- DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataClass.h
- DashSync/shared/Models/Entities/DSAccountEntity+CoreDataClass.m
- DashSync/shared/DashSync.xcdatamodeld/DashSync 21.xcdatamodel/contents
- DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataClass.m
- DashSync/shared/Categories/NSData/NSData+DSMerkAVLTree.h
🚧 Files skipped from review as they are similar to previous changes (8)
- DashSync.podspec
- DashSync/shared/Models/Entities/DSFriendRequestEntity+CoreDataClass.m
- DashSync/shared/Models/CoinJoin/DSTransactionInput+CoinJoin.h
- DashSync/shared/Models/CoinJoin/DSCompactTallyItem.h
- DashSync/shared/DashSync.m
- DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.h
- DashSync/shared/Models/Chain/DSChainConstants.h
- DashSync/shared/Models/CoinJoin/DSCoinControl.h
🧰 Additional context used
🧬 Code Definitions (2)
DashSync/shared/Models/Entities/DSDashpayUserEntity+CoreDataClass.h (1)
DashSync/shared/Models/Identity/DSIdentity+Protected.h (1)
DSIdentity(26-26)
DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.h (1)
DashSync/shared/Models/Derivation Paths/DSDerivationPath.h (4)
chain(158-160)chain(161-163)chain(164-166)chain(172-234)
🔇 Additional comments (307)
DashSync/shared/Categories/NSArray+Dash.m (1)
71-185: Validate memory allocation and deallocation.The methods (e.g.,
ffi_to_vec,ffi_to_vec_of_string,ffi_to_vec_u256, etc.) rely onmallocandstrduporu256_ctorfor allocation, while their corresponding destroy functions are expected to free this memory. If those destroy functions are not consistently invoked, it can lead to memory leaks. Consider verifying that all caller code properly matches these creation and destruction functions for robust memory management.DashSync/shared/Models/Chain/DSChainLock.m (9)
26-26: No concerns regarding the new import.
This addition seems necessary for referencing chain parameters.
51-56: Proper cleanup of the Rust struct.
Releasing_lockwithDChainLockDtorindeallocis correct and prevents memory leaks. Good job.
65-65: Ensure correct handling of potential NULL from rust FFI.
Ifdash_spv_masternode_processor_processing_chain_lock_from_messagecan fail and return NULL, please consider how that scenario is handled downstream to avoid possible dereferences of a NULL pointer.
79-95: Well-structured initializer with safe bridging.
This initializer properly constructs the RustDChainLockfrom the provided block hash and signature, ensuring zeroed placeholders on nil inputs.
97-109: Proper bridging for block hash properties.
Memory is allocated and freed correctly (viau256_dtor). Implementation is clean.
111-116: Signature property accessor looks good.
The code carefully retrieves and frees the Rustu768resource, avoiding leaks.
118-123: Consistent memory handling in signatureData.
Same pattern of retrieving the Rust pointer and freeing it. No issues observed.
128-139: Signature verification logic is straightforward.
Wrapping the logic with#if defined(DASHCORE_MESSAGE_VERIFICATION)is clear, and the fallback returningYESensures no accidental blocking.
175-176: Helpful debug information in the description method.
The format string succinctly provides lock status for logging and debugging.DashSync/shared/Models/Derivation Paths/DSDerivationPath.h (10)
25-27: New imports appear necessary and pose no issues.
They align with newly referenced classes and methods.
48-48: DefiningFEATURE_PURPOSE_COINJOINis clear.
This macro neatly sets the integer value for coinjoin functionality.
50-50: Forward class declarations updated properly.
IncludingDSWalletandDSKeyManageris coherent with usage in the implementation.
73-85: Enum updates for CoinJoin and identities.
AddingDSDerivationPathReference_CoinJoinand renaming identity references look consistent and logical.
97-97: Use of pointer forsigningAlgorithm.
Storing this asDKeyKind *is appropriate for bridging from the underlying cryptographic library. Just ensure proper lifecycle management in the .m file.
111-112: New properties for extended key checks.
hasExtendedPublicKeyandhasExtendedPrivateKeyare descriptive and convenient for quickly verifying key presence.
147-147: Addingdepthfor derivation path hierarchy clarity.
Makes it easy to interpret and display the derivation level.
150-172: Additional method signatures incorporate new parameters gracefully.
They align with the expansions forsigningAlgorithmand reference types without breaking existing usage.
221-229: Extended method to register addresses with context.
Providing a managed object context parameter can help unify database operations.
232-235: Bridging category for index path FFI.
This interface extends the class for integration with the Rust-based crypto library (viadash_spv_crypto_keys_key_IndexPathU256).DashSync/shared/Models/Derivation Paths/DSDerivationPath.m (15)
25-35: New imports align with references in the updated code.
Imports likeDSAccount.h,DSChain+Params.h, andDSDerivationPathFactory.hare necessary for the new functionalities.
59-72:masterIdentityContactsDerivationPathForAccountNumbermethod implementation.
Constructs partial derivation path withFEATURE_PURPOSEandFEATURE_PURPOSE_DASHPAY. Clear purpose and usage.
75-89: Factory method for derivation path with indexes.
Logical parameter order and a well-defined chain reference.
120-148: Initialization with extended public key identifier.
Cleanly retrieves key data from keychain and sets up addresses. Proper fallback usage if the key is not found.
149-194: Comprehensive init linking indexes and memory.
Allocates_hardenedIndexesand cleans up indealloc. Implementation is correct and robust.
220-221: BIP32 indexing logic.
This code sets the correct hardened bit when needed. No off-by-one issues detected.
462-465: Creating a unique derivation path identifier via FFI.
Works well to generate a short hex string.
529-534: Generating extended public key without storing private key.
Straightforward approach returningDMaybeOpaqueKey. This ensures better security by discarding the private key.
570-570: Assertion for signing algorithm consistency.
Ensures the parent and child derivation paths share the same encryption scheme.
582-583: Safe deallocation if_extendedPublicKeyis already initialized.
The call toDMaybeOpaqueKeyDtoravoids leaks.
609-610: Public key retrieval logic is concise.
The code obtains and wraps the public key data according to the storedsigningAlgorithm.
617-620: Checking for extended private key presence.
Relies on keychain data existence. Straightforward usage.
627-629: Registering addresses without direct logic here.
By design, subclasses override for actual usage. No issues.
631-634: Context-based address registration method.
Again, a stub for subclasses. Properly returns nil if unimplemented.
639-655: FFI bridging helpers.
Allocations and deallocations are properly matched with the Rust functions for indexes and booleans. Implementation looks correct.DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataProperties.m (1)
26-26: New dynamic property added for identity associationThis addition of the
identityproperty toDSAssetLockTransactionEntitymakes sense in the context of the CoinJoin feature, allowing asset lock transactions to be associated with an identity.This change is consistent with the overall approach of enhancing identity management throughout the codebase.
DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataClass.h (1)
12-12:Details
❓ Verification inconclusive
Simplified class dependencies
The removal of
DSQuorumEntryEntityfrom the forward declarations indicates a reduction in dependencies, which is good for code maintainability.This change aligns with the broader refactoring observed in the PR where several quorum-related files have been removed.
Verify that all references to
DSQuorumEntryEntityhave been properly removed from the implementation ofDSChainLockEntityto avoid compilation issues:
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to DSQuorumEntryEntity in DSChainLockEntity implementation grep -r "DSQuorumEntryEntity" --include="DSChainLockEntity*.m" DashSync/Length of output: 72
Action Required: Verify DSQuorumEntryEntity Removal in DSChainLockEntity Implementation
The removal of
DSQuorumEntryEntityfrom the forward declarations inDSChainLockEntity+CoreDataClass.happears to simplify class dependencies as intended. An automated search forDSQuorumEntryEntityreferences in the DSChainLockEntity implementation files (using a grep command) returned no matches. However, since the automated output was minimal and might be insufficient for full confidence, please manually verify that all references toDSQuorumEntryEntityhave indeed been removed from the DSChainLockEntity implementation to ensure no compilation issues arise.
- Verify that DSChainLockEntity implementation files (e.g., files matching
DSChainLockEntity*.m) contain no references toDSQuorumEntryEntity.- Confirm that this change aligns with the broader refactoring effort removing related quorum files.
@class DSMerkleBlockEntity, DSChain, DSChainLock, DSChainEntity;DashSync/shared/Categories/NSDate+Utils.h (1)
15-16: New time interval utility methods look goodThese new utility methods for calculating adjusted time intervals are well-named and follow the established pattern for this category.
DashSync/iOS/Models/Managers/Service Managers/Auth/Controllers/DSBasePinViewController.m (1)
32-32:Details
✅ Verification successful
Verify correct usage of DSLocalizedFormat function
The implementation has been changed from using a standard localization approach to using
DSLocalizedFormat. The use ofnilas the second parameter is unusual and should be verified.
🏁 Script executed:
#!/bin/bash # Find the implementation of DSLocalizedFormat to verify correct usage grep -r "DSLocalizedFormat" --include="*.h" --include="*.m" # Check for other usages of this function to confirm the pattern grep -n "DSLocalizedFormat.*nil" --include="*.m"Length of output: 17828
DSLocalizedFormat usage verified—no changes needed
The search confirms that using
nilas the second parameter is a consistent pattern across the codebase (e.g., in NSError+Platform.m, DSIdentitiesManager.m, and others). The implementation in DSBasePinViewController.m follows the established convention, so no modifications are required.DashSync/shared/Models/Entities/DSGovernanceVoteEntity+CoreDataProperties.m (1)
23-23: Verify Core Data model integrity after removing masternode propertyThe
masternodeproperty has been commented out, which removes the dynamic relationship between governance votes and masternodes. This change may impact existing code that relies on this relationship.Please ensure that:
- Any code previously accessing this property has been updated
- Core Data migration is properly handled
- The functionality using
masternodeHashandmasternodeIndex(which are still present) correctly replaces what was previously done with themasternoderelationshipThis change appears to be part of the broader CoinJoin-related refactoring mentioned in the PR description.
DashSync/shared/Models/Entities/DSContractEntity+CoreDataClass.m (1)
10-10: LGTM: Import statement for DSChain headerThe import statement is appropriately added to support the new method implementation.
DashSync/shared/Models/Chain/DSFullBlock.m (1)
21-21: New imports added for chain parameters and coinbase transactionsThe added imports for
DSChain+Params.handDSCoinbaseTransaction.hare appropriate given the file's functionality. TheDSFullBlockclass already works with coinbase transactions in theinitWithCoinbaseTransaction:transactions:previousBlockHash:previousBlocks:timestamp:height:onChain:method, so making this dependency explicit is good practice.Also applies to: 23-23
DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataProperties.h (1)
19-19: Added import for DSBlockchainIdentityEntityThis import addition is appropriate as it's required for the new property being added to the entity.
DashSync/shared/Categories/NSObject+Notification.h (1)
18-18: Simplified import to Foundation frameworkThe change from importing a specific DashSync component to the more fundamental Foundation framework is appropriate for this utility category.
DashSync/shared/Models/Entities/DSAddressEntity+CoreDataClass.h (1)
28-28:Details
✅ Verification successful
Removed DSSimplifiedMasternodeEntryEntity from class declarations
This change aligns with the broader removal of functionality related to simplified masternode entries across the codebase, as mentioned in the PR summary.
However, verify that there are no remaining references to
DSSimplifiedMasternodeEntryEntityin the implementation file that could cause compilation errors.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to DSSimplifiedMasternodeEntryEntity in the implementation rg "DSSimplifiedMasternodeEntryEntity" "DashSync/shared/Models/Entities/DSAddressEntity+CoreDataClass.m"Length of output: 102
Below is an additional re‑verification script that searches the entire repository for any reference to
DSSimplifiedMasternodeEntryEntity. This will help us confirm that no lingering references remain that might cause compilation issues:Please run this script and verify that no occurrences are reported before finalizing the review.
🏁 Script executed:
#!/bin/bash # Perform a repository-wide search for any references to DSSimplifiedMasternodeEntryEntity rg "DSSimplifiedMasternodeEntryEntity"Length of output: 44034
DSAddressEntity Implementation Verified
- The class declaration in
DashSync/shared/Models/Entities/DSAddressEntity+CoreDataClass.hhas been updated to removeDSSimplifiedMasternodeEntryEntity.- A search in
DashSync/shared/Models/Entities/DSAddressEntity+CoreDataClass.mconfirmed that there are no lingering references that could cause compilation issues.- Note that while the repository-wide search shows references to
DSSimplifiedMasternodeEntryEntityin test files and data model definitions, these are outside the scope of this change.DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.h (1)
36-36:Details
❓ Verification inconclusive
Type mismatch with implementation and callers
The method signature has been changed to use
DChainType *instead of the previous type. Ensure that:
- The method implementation in the
.mfile has been updated to match this signature- All callers of this method have been updated to provide the new type
- The new type
DChainTypeis properly defined in the imported headers
🏁 Script executed:
#!/bin/bash # Look for the implementation of this method echo "Searching for implementation of chainEntityForType:checkpoints:inContext:" rg "chainEntityForType:\s*\(DChainType\s*\*\)\s*type" -A 3 # Look for callers of this method to verify they've been updated echo -e "\nSearching for callers of chainEntityForType:checkpoints:inContext:" rg "chainEntityForType:" --include "*.m" -A 1Length of output: 1350
Action Required: Verify Implementation and Caller Updates
The updated method signature in the header now uses
DChainType *as expected. However, our automated search for the corresponding implementation and for all call sites encountered an error with the grep flags (i.e. the--includeflag issue). This means we couldn’t automatically confirm that:
- The implementation in the corresponding
.mfile has been updated to match this signature.- All callers have been updated to pass a
DChainType *.Please manually verify that these changes have been applied consistently throughout the codebase. Also, ensure that the new type
DChainTypeis correctly defined in the imported headers.DashSync/shared/Categories/NSData/NSData+DSHash.m (2)
26-26:Details
✅ Verification successful
Import consistency across codebase
The import has been changed from "dash_shared_core.h" to "dash_spv_apple_bindings.h". Ensure that this change is consistent across the entire codebase to avoid unexpected behavior or build issues.
🏁 Script executed:
#!/bin/bash # Check if there are any files still using the old import echo "Files still using the old import:" rg "#import\s+\"dash_shared_core\.h\"" -l # Check which files are using the new import echo -e "\nFiles using the new import:" rg "#import\s+\"dash_spv_apple_bindings\.h\"" -lLength of output: 2166
Import update verified across the codebase
- No instances of
#import "dash_shared_core.h"were found.- The new import
#import "dash_spv_apple_bindings.h"is being used consistently (including inDashSync/shared/Categories/NSData/NSData+DSHash.m).
32-38:Details
❓ Verification inconclusive
Verify blake3 hashing equivalence after implementation change
The
blake3Dataimplementation has been completely changed to use Rust FFI functions instead of the previous implementation. While the code itself looks correct with proper memory management (creating the slice, deallocating the result), you should verify that:
- The hash results are identical to the previous implementation
- There are unit tests that validate the correctness of this hashing function
- Performance characteristics are at least equivalent to the previous implementation
🏁 Script executed:
#!/bin/bash # Check if there are tests for blake3 hashing echo "Searching for blake3 tests:" rg "test.*blake3|blake3.*test" --include "*.m" # Look for existing benchmark code echo -e "\nSearching for blake3 benchmarks:" rg "benchmark.*blake3|blake3.*benchmark" --include "*.m"Length of output: 1137
Action Required: Verify Blake3 Hashing Correctness and Test Coverage
Please manually run the updated script above to confirm that unit tests and benchmark cases for the new Rust FFI–based
blake3Dataimplementation exist. In addition, ensure that:
- The hash output remains equivalent to the previous implementation.
- Adequate unit tests are present to validate the correctness of the implementation.
- Performance characteristics are comparable to—or better than—the original version.
Once you’ve confirmed these points, update or add tests/benchmarks as needed.
🏁 Script executed:
#!/bin/bash # Corrected search for blake3 tests and benchmarks in Objective-C (.m) files using the -g flag instead of --include. echo "Searching for blake3 tests (in .m files):" rg "test.*blake3|blake3.*test" -g "*.m" echo -e "\nSearching for blake3 benchmarks (in .m files):" rg "benchmark.*blake3|blake3.*benchmark" -g "*.m"Length of output: 417
Action Required: Verify Blake3 Hashing Equivalence, Test Coverage, and Performance
The new Rust FFI–based implementation in
DashSync/shared/Categories/NSData/NSData+DSHash.mappears correct in terms of memory management. However, our repository search for unit tests and benchmark code (using patterns in.mfiles) returned no results. This leaves the following action items:
- Hash Equivalence: Manually verify that the new
blake3Dataimplementation produces identical hash outputs compared to the previous implementation.- Unit Tests: Ensure there are unit tests covering this functionality. If none exist, please add tests to validate the correctness of the Blake3 hashing.
- Performance: Confirm that performance (and resource usage) is at least equivalent to the original implementation. Benchmarks might be necessary if performance is a critical factor.
Please perform a manual review to confirm these aspects and update the test coverage or benchmarking as needed.
DashSync/shared/Categories/NSData/NSData+Dash.h (1)
249-250: Well-defined method signatures for CoinJoin functionalityThese new methods appear to be part of the CoinJoin feature implementation, providing necessary script generation capabilities:
creditBurnScriptPubKeyForAddress:forChain:- Likely creates a script for burning credits in the CoinJoin processassetLockOutputScript- Generates a script for locking assetsThe signatures follow the existing naming conventions in the category and are properly placed alongside other script-related methods.
DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataProperties.m (2)
18-18: Adding cycleHash property for InstantSendLock entityThe addition of the
cycleHashproperty to the InstantSendLock entity suggests a change in how instant transactions are identified or verified, likely related to the CoinJoin implementation. This aligns with the broader architectural changes in the PR.
21-21:Details
❓ Verification inconclusive
Replacing quorum with version property
Changing from
quorumtoversionindicates a significant model change in how InstantSendLock entities are structured. This appears to be part of a broader update to how transaction verification works in the CoinJoin context.
🏁 Script executed:
#!/bin/bash # Check for any references to the old quorum property that might need updating rg -A 3 "quorum.*DSInstantSendLockEntity" --type objcLength of output: 55
Below is a revised review comment that requests additional verification to ensure no lingering references to the old
quorumproperty remain:
Replacing
quorumwithversionin DSInstantSendLockEntityThe change from using
quorumtoversionrepresents an important update in the InstantSendLock entity model (aligned with modifications in the CoinJoin transaction verification flow). Although the previous search returned no output, that result alone isn’t fully conclusive. Please run the following additional shell scripts to double-check that no remnants of the oldquorumproperty exist in the codebase:#!/bin/bash # Search for any property declaration related to 'quorum' rg -A 3 "@dynamic quorum" --type objc #!/bin/bash # Search for any usage of 'quorum' within files related to DSInstantSendLockEntity rg "quorum" --type objcOnce you confirm that these searches yield no references to
quorum, we can be confident that the replacement is complete.DashSync/shared/Categories/NSObject+Notification.m (2)
3-3: Update copyright yearThe copyright year has been updated to 2025, which is future-dated compared to the current year 2024.
Consider updating to the correct year (2024) unless there's a specific policy for using future years.
18-29: New notification helper methodGood implementation of a notification helper that follows best practices:
- Dispatches to the main queue for UI safety
- Provides a clean, simplified API for posting notifications
- Makes the notification object nil which is appropriate for app-wide notifications
This will help standardize notification handling throughout the codebase.
DashSync/shared/Models/Governance/DSGovernanceVote.h (3)
8-10: Updated imports for new bindingsReplacing
dash_shared_core.hwithdash_spv_apple_bindings.hand updating the key manager import align with the PR objective of updating the DashSharedCore version to 0.5.0. These changes reflect the migration to the new API.
49-49: Updated key type for signingThe parameter type change from
OpaqueKey *toDOpaqueKey *in the signing method is consistent with other type changes in this file. This ensures compatibility with the updated DashSharedCore version.
37-37:Details
❓ Verification inconclusive
Updated masternode type
Changing from
DSSimplifiedMasternodeEntry *toDMasternodeEntry *reflects the adoption of new types from the updated DashSharedCore dependency. This is an important structural change that will affect how masternode data is represented.
🏁 Script executed:
#!/bin/bash # Check if DMasternodeEntry is properly defined in the imported headers rg -A 3 "DMasternodeEntry" --type-not m --type-not mmLength of output: 79
🏁 Script executed:
#!/bin/bash # Re-run search for the definition of DMasternodeEntry by checking for its Objective-C interface declaration. rg -n '@interface DMasternodeEntry'Length of output: 37
Action Required: Verify External Dependency Import of DMasternodeEntry
The updated type change from
DSSimplifiedMasternodeEntry *toDMasternodeEntry *appears to follow the new dependency update in DashSharedCore. However, our automated searches did not locate a direct definition ofDMasternodeEntrywithin the repository. Please manually verify that the updated type is properly imported from DashSharedCore and that all unchanged references to this dependency are accurate.
- Confirm that the header defining
DMasternodeEntry(likely provided by DashSharedCore) is correctly imported.- Ensure that there are no lingering references to
DSSimplifiedMasternodeEntrythat might conflict with this change.DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataClass.m (1)
105-113: Effective lazy loading implementation for masternodesThe new
loadLocalMasternodesInContext:onChainEntity:method provides a clean way to load masternode entities filtered by chain. The method properly uses a predicate to filter by chain entity and the comment about lazy loading makes the intention clear.DashSync/shared/Models/Entities/DSContractEntity+CoreDataClass.h (3)
11-11: Good addition of utility importAdding the NSManagedObject+Sugar.h import is appropriate as it likely provides Core Data helper methods that will be needed for the new entity creation method.
13-13: Updated class forward declarationAdding DSChain to the forward declarations is necessary to support the new method parameter.
19-21: Well-structured factory methodThe new factory method follows good Objective-C patterns for entity creation and properly includes all necessary parameters (identifier, chain, and context).
DashSync/shared/Categories/NSData/NSData+Dash.m (3)
36-36: Updated import for DashSharedCore dependencyThe import has been changed from
dash_shared_core.htodash_spv_apple_bindings.h, which aligns with the DashSharedCore update mentioned in the PR description.
1576-1581: Clean implementation of asset lock output scriptThe
assetLockOutputScriptmethod provides a simple and clear way to create an OP_RETURN script for asset lock operations, which is part of the new CoinJoin functionality.
1583-1587:Details
✅ Verification successful
Verify the implementation of creditBurnScriptPubKeyForAddress helper method
The method calls
appendCreditBurnScriptPubKeyForAddress:forChain:but we need to ensure this method exists and is properly implemented elsewhere.
🏁 Script executed:
#!/bin/bash # Check for the implementation of appendCreditBurnScriptPubKeyForAddress:forChain: rg -A 3 "appendCreditBurnScriptPubKeyForAddress:" --type objcLength of output: 1670
Implementation Verified: The helper method
appendCreditBurnScriptPubKeyForAddress:forChain:is correctly declared inDashSync/shared/Categories/NSData/NSMutableData+Dash.hand implemented inDashSync/shared/Categories/NSData/NSMutableData+Dash.m. The grep output confirms that the method is present and properly called fromcreditBurnScriptPubKeyForAddressForAddress:forChain:inNSData+Dash.m.
- Declaration: Present in
NSMutableData+Dash.h- Implementation: Found in
NSMutableData+Dash.m(usingaddress.base58checkToDataappropriately)- Usage: Correctly utilized in the
creditBurnScriptPubKeyForAddress:forChain:method without any discrepanciesNo issues were found with the implementation.
DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath+Protected.h (2)
17-18: LGTM: Added method for standardized gap limit settingsThe addition of
defaultGapSettingsmethod returning aDSGapLimitobject represents a good enhancement to the derivation path system, moving away from rigid integer-based gap limits to a more flexible, object-oriented approach.
19-19: LGTM: Added context-aware method for address loadingThe new
loadAddressesInContext:method improves thread safety and Core Data integration by explicitly requiring a managed object context parameter, making the address loading process more predictable and manageable across different threading scenarios.DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataProperties.h (1)
19-20: LGTM: Enhanced InstantSendLock with versioning and cycle supportThe addition of
cycleHashandversionproperties improves the InstantSendLock entity by adding support for versioning and cycle-based validation, which appears to be part of an update to the instant transaction locking mechanism.Verify that all code handling InstantSendLock entities has been updated to account for these new properties:
#!/bin/bash # Search for code that might need updating with the new InstantSendLock properties echo "Searching for code using DSInstantSendLockEntity..." rg "DSInstantSendLockEntity" -A 5DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataClass.h (2)
12-12: LGTM: Updated class references for identity system refactoringThe class reference update from
DSBlockchainIdentitytoDSIdentityaligns with the broader identity system refactoring across the codebase, generalizing the concept of identity beyond blockchain-specific implementations.
18-18: LGTM: Renamed method for consistency with identity system refactoringThe method rename from
blockchainIdentitytoidentitymaintains consistency with the identity system refactoring and provides a cleaner, more generic API.Ensure that all call sites have been updated:
#!/bin/bash # Search for potential missed references to the old method name echo "Searching for potentially missed references to blockchainIdentity method..." rg "blockchainIdentity\]|blockchainIdentity\s*\]" --type objcDashSync/shared/Models/Governance/DSGovernanceObject.m (2)
10-10: LGTM: Added import for chain parametersAdding the import for
DSChain+Params.hsupports the updated hash computation that now requires access to chain parameters.
302-305: LGTM: Improved hash computation and loggingThe implementation now uses the more efficient
dashcore_hash_types_Sha256dHash_innerfunction for hash computation andu256_hexfor string representation, which is consistent with modern hash handling practices in the codebase.Make sure to free memory properly after hash operations to prevent memory leaks, which is correctly done with
u256_dtor(entry_hash).DashSync/shared/Models/Entities/DSBlockchainIdentityKeyPathEntity+CoreDataProperties.h (1)
9-9: Header import rename looks consistent.
No immediate issues found with using"DSIdentity.h"instead of the old"DSBlockchainIdentity.h", given the broader renaming throughout the codebase.DashSync/shared/Models/CoinJoin/DSInputCoin.h (1)
18-24: Imports appear consistent.
All imported headers (e.g.,DSKeyManager,DSTransactionOutput) are referenced by the class or its methods. No duplication or apparent missing imports detected.DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath+Protected.h (1)
18-19: Method name updates to align with the identity-based naming.
Renaming the methods fromblockchainIdentity*toidentity*appears consistent with the overall shift toward identity-based classes. Verify downstream references to these methods have also been updated accordingly.DashSync/shared/Models/Governance/DSGovernanceVote.m (3)
8-8: New import validates usage of the underlying C/Rust bridging functions.
Thedash_spv_apple_bindings.hheader provides low-level bridging. Since the methods in lines 143-144 rely on external C infrastructural calls, this inclusion is necessary.
11-11: Import of DSChain+Params.h for protocolVersion usage.
IncludingDSChain+Params.his logical given this file checksself.chain.protocolVersion.
22-22: Confirm memory semantics for masternode property.
Declaring@property (nonatomic, assign) DMasternodeEntry *masternode;is unusual for an Objective-C object pointer. IfDMasternodeEntryis not anNSObject,assigncan be valid. Otherwise, astrongreference is typically safer. Verify thatDMasternodeEntryis indeed a non-Objective-C type or that this usage is intentional.DashSync/shared/Categories/NSIndexPath+FFI.h (1)
19-19: Confirm updated import feasibility.
The switch from"dash_shared_core.h"to"dash_spv_apple_bindings.h"signals a dependency or interface change. Ensure that any previously used declarations remain accessible under the new header and that no breaking changes occur.Would you like me to run a search script to confirm that all required symbols are present in
dash_spv_apple_bindings.h?DashSync/shared/Models/Entities/DSDashpayUserEntity+CoreDataClass.h (1)
30-30: Verify the new class reference.
The introduction ofDSIdentityin the@classlist suggests a dependency on new identity-based data types. Confirm that all references toDSIdentityare compiled successfully and used consistently throughout the codebase.DashSync/shared/Models/CoinJoin/DSInputCoin.m (4)
1-17: License header looks good.
All standard licensing clauses are present and correctly formatted.
22-31: Initialize outpoint details thoroughly.
The constructor captures and stores the transaction’s outpoint and output data effectively. This approach cleanly separates configuration from usage, ensuring readability.
33-39: Address the outpoint hash endianness TODO.
The comment warns about reversing the outpoint hash. Failing to handle it consistently (if required by the underlying Core or RPC usage) may cause incorrect references for transaction inputs.Would you like me to generate a verification script to search for usage patterns of
DOutPointCtorUand confirm whether a reversed byte order is typically expected?
40-43: Memory deallocation routines appear correct.
Time complexity for de-allocation is minimal, and the usage ofDInputCoinDtoris straightforward. This ensures a clean teardown.DashSync/shared/Categories/NSData/NSData+DSMerkAVLTree.m (2)
23-23: Dependency update to use apple bindingsChanging from
dash_shared_core.htodash_spv_apple_bindings.his consistent with the PR's goal of updating DashSharedCore.
27-74: Clarify the fate of commented-out methodThis entire method has been commented out rather than removed. Is this functionality being replaced elsewhere, or is this a temporary change?
If this functionality has been replaced by something in the updated DashSharedCore, consider removing this code entirely and adding a comment referencing the new implementation. If this is temporary, add a TODO comment explaining the plan.
DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataClass.m (3)
9-9: Consistent terminology update for identity classesThe import change from
DSBlockchainIdentity+Protected.htoDSIdentity+Protected.his part of the broader identity management refactoring in this PR.
18-20: Consistent variable naming with identity terminologyVariable renaming maintains consistency with the identity terminology changes.
25-26:Details
❓ Verification inconclusive
Method signature updated for identity refactoring
The method has been updated to return
DSIdentity*instead ofDSBlockchainIdentity*, consistent with the broader refactoring of identity-related classes.Ensure that all callers of this method have been updated to handle the new return type. Consider running a search across the codebase:
🏁 Script executed:
#!/bin/bash # Search for any places that might still reference blockchainIdentity # instead of identity in relation to this entity rg "blockchainIdentity.*DSBlockchainIdentityEntity" --type objcLength of output: 65
Action Required: Verify Caller Updates for Identity Refactoring
The method now correctly returns a
DSIdentity*in line with the refactored identity system. Our initial automated search for references toblockchainIdentityalongsideDSBlockchainIdentityEntitydid not yield any results, suggesting that outdated references might have been removed. However, since the search output was minimal, please manually verify that all callers of this method have been updated to accommodate the new return type.
- Confirm that there are no remaining code sections expecting a
DSBlockchainIdentity*from this method.- Double-check any calls that might still reference legacy naming or types.
DashSync/shared/Models/Chain/DSMerkleBlock.h (1)
55-59: Improved method signature formattingBreaking the long initializer method signature into multiple lines significantly improves readability without changing functionality.
DashSync/shared/Categories/NSSet+Dash.m (1)
55-76:Details
❓ Verification inconclusive
New category for FFI integration with Vec_u8_32
This new category adds important FFI conversion methods between Objective-C NSSet<NSData*> and what appears to be a Rust-like Vec_u8_32 structure, likely supporting the CoinJoin functionality.
The implementation includes proper memory management, but consider:
- Are there unit tests for these conversion methods?
- Check if error handling is needed for edge cases (e.g., malformed input data)
- Verify thread safety if these methods might be called from multiple threads
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new methods rg "test.*ffi_from_vec_u256|test.*ffi_to_vec_u256" --type objcLength of output: 64
Please verify the following for the new FFI conversion methods in
DashSync/shared/Categories/NSSet+Dash.m:
- The search for unit tests targeting
ffi_from_vec_u256andffi_to_vec_u256did not return any results—please confirm if proper tests are in place or add them if missing.- Consider whether additional error handling is needed for malformed input data.
- Verify that these methods are thread-safe if they may be called concurrently.
DashSync/shared/Models/CoinJoin/DSTransactionInput+CoinJoin.m (2)
25-34: Add memory allocation validation for robustness.The
ffi_mallocmethod allocates memory but doesn't check for allocation failures. This could lead to crashes if memory allocation fails.Implement checks after memory allocations to handle failure cases properly:
- (DTxIn *)ffi_malloc { DOutPoint *outpoint = DOutPointCtorU(self.inputHash, self.index); + if (!outpoint) { + NSLog(@"Failed to allocate memory for DOutPoint"); + return NULL; + } DScriptBuf *script; if (self.inScript) script = DScriptBufCtor(bytes_ctor(self.inScript)); else script = DScriptBufCtor(bytes_ctor(self.signature)); + if (!script) { + NSLog(@"Failed to allocate memory for DScriptBuf"); + DOutPointDtor(outpoint); + return NULL; + } return DTxInCtor(outpoint, script, self.sequence); }
53-55: LGTM!The null check before freeing memory is a good practice to avoid potential crashes.
DashSync/shared/Models/Derivation Paths/DSMasternodeHoldingsDerivationPath.m (2)
28-36: Update looks good - switched to direct coinType access.The code now uses
chain.coinTypedirectly instead of a function call, which is cleaner and more maintainable. The parameter names have been updated for clarity as well.
39-41: LGTM!The implementation of
defaultGapSettingsreturns a reasonable gap limit value of 5, which should provide a good balance between address reuse prevention and scanning performance.DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath+Protected.h (2)
18-19: LGTM!Adding the import for the main class header is a good practice to make the category extension self-contained.
24-26: Method renaming improves clarity.The method names have been updated to use "identity" instead of what was likely "blockchainIdentity", making them more concise while maintaining clarity.
DashSync/shared/Categories/NSIndexPath+FFI.m (2)
20-20: Category naming clarified.Renaming the category from
FFItoVec_u32more specifically describes its purpose, aligning with the actual data structure being handled.
35-37: LGTM!The implementation of
ffi_destroycorrectly handles the deallocation of the Vec_u32 reference.DashSync/shared/Models/Entities/DSAssetLockTransactionEntity+CoreDataClass.m (5)
43-45: Improved credit outputs handling approach.Replacing dynamic adjustment with a clear-and-repopulate approach is cleaner and less error-prone. This eliminates potential issues with inconsistent states when modifying ordered sets.
46-56: Credit outputs creation looks good.The implementation properly creates and populates DSTxOutputEntity instances with all necessary attributes from the transaction outputs.
58-62: Important separation of credit outputs from base outputs.This ensures proper transaction structure by preventing credit outputs from being counted twice. The explicit comment is helpful for future maintenance.
71-72: Added version assignment for special transactions.Setting the transaction version to SPECIAL_TX_VERSION ensures proper handling of this special transaction type.
74-86: Improved credit outputs collection in transactionForChain:Using a temporary array before assigning to tx.creditOutputs is cleaner than directly manipulating the destination collection. This change makes the code more maintainable and less error-prone.
DashSync/shared/Models/Chain/DSChain+Protected.h (2)
90-90:Details
❓ Verification inconclusive
Parameter type change for masternode entries.
Changed from
NSArray *to the more specificDMasternodeEntryList *type. This provides better type safety but may require updates to calling code.
🏁 Script executed:
#!/bin/bash # Check for calls to updateAddressUsageOfSimplifiedMasternodeEntries: # that might need updating rg "updateAddressUsageOfSimplifiedMasternodeEntries:" --glob "*.m"Length of output: 328
Parameter Type Update Verified – Please Confirm Caller Variable Types
The method signature in DashSync/shared/Models/Chain/DSChain+Protected.h now correctly uses the more specific
DMasternodeEntryList *parameter. Our search confirms that this method is implemented in DSChain.m and called in DSDashSharedCore.m. Please ensure that the variable passed (i.e.masternodes) in the calling code is indeed an instance ofDMasternodeEntryList *to maintain type safety across the codebase.
116-116:Details
❓ Verification inconclusive
Method resetLastSyncBlock appears unchanged.
Although marked as modified, the signature appears identical to before. Verify if there are actual implementation changes in the .m file.
🏁 Script executed:
#!/bin/bash # Check implementation of resetLastSyncBlock in DSChain+Protected.m fd "DSChain\\+Protected.m" | xargs cat | awk '/resetLastSyncBlock/,/^}/'Length of output: 73
I'll run a new verification script to check that the DSChain+Protected.m file exists and to locate any occurrence of “resetLastSyncBlock” in that file. This will help confirm whether the method's implementation was updated or if its body remains unchanged.
🏁 Script executed:
#!/bin/bash # List the DSChain+Protected.m file(s) to ensure they exist. fd "DSChain+Protected.m" # Search for the method implementation/declaration of resetLastSyncBlock in DSChain+Protected.m with line numbers. grep -n "resetLastSyncBlock" DSChain+Protected.m || echo "Method 'resetLastSyncBlock' not found in DSChain+Protected.m"Length of output: 263
Verify the Location and Changes for
resetLastSyncBlockThe header declaration in
DashSync/shared/Models/Chain/DSChain+Protected.hremains unchanged, but our automated search did not find a correspondingDSChain+Protected.mfile to compare the implementation. Please verify manually that the implementation ofresetLastSyncBlockis updated as intended—either by checking if it exists under an alternate name or location, or by confirming that no changes were needed in the implementation.DashSync/shared/Models/Entities/DSLocalMasternodeEntity+CoreDataClass.h (1)
12-12: Simplified forward declarations.The removal of DSSimplifiedMasternodeEntryEntity from forward declarations suggests a reduction in dependencies, which is good for code maintainability.
DashSync/shared/Models/CoinJoin/DSCoinControl.m (3)
58-73: Effective UTXO selection checking methods.The methods to check if UTXOs are selected are implemented correctly, using value comparison rather than reference comparison.
75-81: Clear CoinJoin usage control.These methods provide a simple interface for enabling/disabling CoinJoin functionality by setting the appropriate coin type.
83-88: UTXO selection prevents duplicates.The method properly checks for duplicates before adding a new UTXO to the selected set, preventing potential issues with duplicate entries.
DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.h (2)
1-16: License header correctly implemented.The MIT license header is properly formatted and includes all necessary information.
35-43: LGTM! Factory method follows Objective-C naming conventions.The factory method has a clear name that follows Objective-C naming conventions and makes it easy to understand its purpose.
DashSync/shared/Models/Entities/DSInstantSendLockEntity+CoreDataClass.m (2)
34-45: Use of property accessors looks good.The method correctly accesses properties to set attributes from the transaction lock.
47-60: Updated initialization with new parameters looks good.The method
instantSendTransactionLockForChain:has been properly updated to match the new signature of theDSInstantSendTransactionLockinitializer, including the newcycleHashparameter.DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.h (1)
57-57:Details
❓ Verification inconclusive
Verify the impact of large gap limit on performance.
The CoinJoin gap limit of 400 is significantly larger than other gap limits used in the codebase. This could potentially impact performance during address derivation and scanning.
I recommend verifying that the large gap limit doesn't cause performance issues, especially on mobile devices. Consider running performance tests to measure the impact of scanning for 400 addresses compared to the standard gap limit of 100.
🏁 Script executed:
#!/bin/bash # Look for existing performance considerations or tests related to gap limits rg -i "performance.*gap limit|gap limit.*performance" --type-not binaryLength of output: 104
Action required: Verify the performance impact of the extended gap limit on mobile devices.
- In
DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.h(line 57), the new coinJoin derivation path method now uses a gap limit of 400, which is considerably higher than the typical limit of 100.- Because scanning for 400 addresses instead of 100 could incur additional processing overhead—especially on resource-constrained mobile devices—it’s important to ensure this change does not cause any significant performance degradation.
- Please perform manual performance tests that compare the scanning times and resource usage for the 400-address gap limit versus the standard 100-address limit.
DashSync/shared/Libraries/Logs/CompressingLogFileManager.m (3)
121-126: Confirm thread safety when adding files to compress while compression is ongoing.
The loop checks iflogFileInfois archived or starts with “processor”, but changes to that array while one file is being compressed might cause missed entries or duplicates if there's no synchronization. Validate that you aren't simultaneously mutatingsortedLogFileInfos.
253-260: Check for file protection attributes if background tasks are not supported.
While you setNSFileProtectionKeyto match the original file, ensure the app is correctly configured for background tasks to avoid potential compression failures when the device is locked. If background execution is not guaranteed, consider an alternate approach.
317-444: Validate robustness of the compression loop to handle partial reads/writes.
Be sure to handle edge cases (e.g., read returns 0 unexpectedly, or deflate buffers fill up unexpectedly). The current logic appears correct, but thorough testing is advised to ensure you don't silently drop data.DashSync/shared/Models/CoinJoin/DSCoinJoinBalance.m (1)
50-57: Verify memory ownership rules for FFI bridging.
Ensure that the lifecycle ofDBalance *is clearly bounded, especially if references to the same object exist elsewhere. Confirm that callingdash_spv_coinjoin_models_balance_Balance_destroyis the sole mechanism for releasing memory.DashSync/shared/Models/CoinJoin/DSTransaction+CoinJoin.m (2)
40-41: Clarify Hash Endianness.There's a
// TODO: check if it's reversedcomment. CoinJoin transaction processing often requires reversed hashes for certain operations, but this depends on how the rest of the DashCore code handles endianness.If you wish, I can generate a script to locate other references to
dashcore_hash_types_Txid_inneror usage in the repository to confirm the expected endianness. Let me know if you'd like me to proceed.
60-60: Confirm handling ofNSNullfor non-standard outputs.Storing
NSNullfor non-standard outputs might be beneficial if you have logic to recognize OP_RETURN or similar scripts later. Double-check that downstream code gracefully processes[NSNull null]to avoid crashes.DashSync/shared/Models/Entities/DSChainLockEntity+CoreDataClass.m (1)
21-39: EnsurelastChainLocklogic is correct.When updating
chainEntity.lastChainLock, confirm that the height comparison uses the correct block height from the chainLock. IfchainEntity.lastChainLock.merkleBlock.heightis incorrectly maintained, the chain's final chain lock might not be updated accurately.DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataClass.m (5)
24-24: New import added for checkpoint functionality.The addition of the DSChain+Checkpoint.h import aligns with the overall architecture changes supporting the CoinJoin feature. This import provides access to checkpoint-related functionality that's likely needed by the updated methods in this class.
27-27: New import added for chain lock functionality.The import of DSChainLock.h provides access to chain lock functionality that will be used with the merkle block entity. This aligns with the CoinJoin updates in this PR.
177-184: Good addition of utility method for checking block existence.This new method provides a clean way to check if blocks with a specific hash exist in the context, which is helpful for the CoinJoin implementation. The implementation correctly uses performBlockAndWait for thread safety with Core Data.
166-171:Details
✅ Verification successful
Parameter type changed and simplified assignment.
The method now accepts NSData directly for blockHash instead of UInt256, and the assignment has been simplified by directly using the parameter rather than converting it. This change improves code consistency and performance.
🏁 Script executed:
#!/bin/bash # Check for any calls to this method with UInt256 that might need updating rg -A 4 "createMerkleBlockEntityForBlockHash:" --glob "*.m" --glob "*.h" | grep -v "NSData"Length of output: 1228
Verified: Simplified blockHash parameter and assignment confirmed
The code now correctly accepts an NSData blockHash, and our grep check confirms there are no residual UInt256 usages in calls to this method. The changes improve both consistency and performance without impacting other parts of the code.
152-153:Details
❓ Verification inconclusive
Method signature updated to use NSData instead of UInt256.
The method now accepts a direct NSData object for blockHash instead of UInt256, which improves consistency and reduces type conversions throughout the codebase. This is a good modernization that aligns with the overall refactoring.
🏁 Script executed:
#!/bin/bash # Check for any calls to this method with UInt256 that might need updating rg -A 2 "merkleBlockEntityForBlockHash:" --glob "*.m" --glob "*.h" | grep -v "NSData"Length of output: 1007
Updated method signature using NSData verified.
The refactored methodmerkleBlockEntityForBlockHash:inContext:now accepts anNSDataobject for the block hash, which removes unnecessary type conversions and improves consistency. A review of usage inDSChainLockEntity+CoreDataClass.mconfirms that call sites are correctly updated (e.g., passingchainLock.blockHashData).
- Note: The header still declares a variant (
merkleBlockEntityForBlockHashFromCheckpoint:) that accepts aUInt256parameter. Please verify whether this endpoint should be similarly modernized for consistency or if its current signature is intentional.Overall, the modernization in this method is solid; just double-check the consistency across similar APIs.
DashSync/shared/Models/Entities/DSDerivationPathEntity+CoreDataClass.m (5)
39-40: Improved method signature formatting.The method signature has been reformatted for better readability by splitting the parameters across multiple lines. This is a good practice for methods with long signatures.
63-67: Enhanced readability of method call.The method call has been reformatted for better readability by splitting the parameters across multiple lines, making the code easier to understand and maintain.
82-84: Improved method signature formatting.The method signature has been reformatted for better readability by splitting the parameters across multiple lines, which is a good practice for methods with many parameters.
105-109: Enhanced readability of method call.The method call has been reformatted with parameters on separate lines, making the code easier to read and maintain.
71-71:Details
✅ Verification successful
Updated property names in predicate.
The predicate now references
contactSourceIdentityUniqueIdandcontactDestinationIdentityUniqueIdinstead of the previous blockchain-prefixed identifiers. This change reflects an architectural shift in how identities are managed in the codebase.
🏁 Script executed:
#!/bin/bash # Verify that these property names exist in the DSIncomingFundsDerivationPath class rg "contactSourceIdentityUniqueId|contactDestinationIdentityUniqueId" --glob "DSIncomingFundsDerivationPath.*"Length of output: 1897
Property Name Updates Verified
The updated predicate now correctly references
contactSourceIdentityUniqueIdandcontactDestinationIdentityUniqueId. Our verification confirms that these properties exist in theDSIncomingFundsDerivationPathclass, aligning with the architectural changes. No further issues were found.DashSync/shared/Models/Chain/DSCheckpoint.m (3)
49-49: Simplified string assignment.The code now directly uses a string literal with the hexToData and reverse methods, rather than using a more verbose string creation process. This makes the code more concise and easier to read while maintaining the same functionality.
111-125: Improved method signature and implementation formatting.The method signature and implementation have been reformatted for better readability by placing each parameter on its own line. This is a good practice for methods with many parameters and makes the code easier to maintain.
129-136: Enhanced readability of method implementation.The method implementation has been reformatted with parameters on separate lines, improving code readability without changing functionality. This is consistent with the formatting style applied to other methods in this PR.
DashSync/shared/Models/Chain/DSCheckpoint.h (2)
51-57: Improved method declaration formatting.The method declaration has been reformatted with parameters on separate lines, which significantly improves readability for this method with many parameters.
59-59: Added new method for creating checkpoints from blocks.This new method provides a convenient way to create checkpoints directly from DSBlock objects with specific options. This is a useful addition that complements the existing API and likely supports the CoinJoin functionality being implemented.
DashSync/shared/Models/Chain/DSChainLock.h (3)
26-26: Header import changes appear correct.
No issues identified.
31-31: Forward declaration usage
Using a forward declaration forDSChainis a standard practice to avoid circular imports. No concerns.
38-40: Properties for block hash and signature
AddingblockHashDataandsignatureDataimproves clarity by using NSData. This is a good approach.DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.m (5)
25-26: Imports updated to adopt new dash_spv_apple_bindings
This approach is suitable if you're leveraging new Apple bindings from dash_spv. No issues.
128-130: Method signature for chain entity
The convenience constructor with parameters(dash_spv_crypto_network_chain_type_ChainType *)typeandNSArray *checkpointsis well-defined. No further issues identified.
132-132: Devnet version retrieval
dash_spv_crypto_network_chain_type_ChainType_devnet_version(type)usage is consistent with new bindings.
58-58:Details
❓ Verification inconclusive
Pointer usage for the chain type
Declaring__block dash_spv_crypto_network_chain_type_ChainType *type;is valid, but ensure the pointer remains valid beyond this scope.
🏁 Script executed:
#!/bin/bash # Attempt to find references to the chain_type pointer usage rg -A 5 $'dash_spv_crypto_network_chain_type_ChainType \\*type'Length of output: 686
Ensure the
__blockpointer's lifetime is managed appropriately
The search confirms that the declaration of__block dash_spv_crypto_network_chain_type_ChainType *type;appears alongside other temporary
__blockvariables. While the declaration itself is valid, please verify that if this pointer is referenced beyond the immediate scope—especially within asynchronous blocks—you have the correct mechanisms (such as strong references) to maintain its validity. This precaution remains advisory unless further context shows improper usage.
49-49:Details
✅ Verification successful
Assigning self.type from chainType
Ensure thatchain.chainTypeis valid and consistent with the newdash_spv_crypto_network_chain_type_ChainType_indexfunction.
🏁 Script executed:
#!/bin/bash # Validate usage of chain.chainType in the codebase to ensure it's properly set rg -A 15 $'\.chainType'Length of output: 65651
Chain Type Assignment Validated
After verifying the widespread usage of
chain.chainTypeacross the codebase—from tests to utility functions—and comparing it with the behavior expected fromdash_spv_crypto_network_chain_type_ChainType_index, it appears that setting:self.type = dash_spv_crypto_network_chain_type_ChainType_index(chain.chainType);is consistent with how similar conversions and validations are performed elsewhere in the project. No discrepancies were detected in how the chain type is passed and handled.
- Multiple references in critical files confirm that
chain.chainTypeis used correctly (e.g., when generating addresses and handling keys).- The conversion function appears to align with project conventions, and similar calls in other modules (such as in test cases and chain parameter setups) follow the same pattern.
No further modifications are needed here.
DashSync/shared/Models/Chain/DSChain+Transaction.h (6)
1-15: License header
Looks consistent with standard Dash Core Group licensing.
18-21: Imports
Importing DSChain.h and DSTransaction.h is appropriate for bridging transaction functionality.
24-24: Category Definition
The category namingDSChain (Transaction)is clear and well-organized.
34-35: Amount calculation
The method name and doc are consistent. Good usage ofamountReceivedFromTransaction:.
40-41: Local reference check
Method name and doc are sufficiently descriptive. No issues found.
45-47: Protected method usage
registerSpecialTransaction:saveImmediately:andtriggerUpdatesForLocalReferences:appear well-defined for internal usage.DashSync/shared/DSDashSharedCore.m (5)
297-316:Details
❓ Verification inconclusive
Check if more explicit synchronization is needed.
Thenotify_sync_state_caller()method synchronizes onsyncInfo, but also interacts withcore.chain.chainManagerand external states. If concurrency arises from multiple calls on different threads, consider whether additional locks or atomic operations are needed to ensure data consistency.Could you run a concurrency test to confirm there are no race conditions? For instance:
Review all call sites to ensure consistent synchronization.
🏁 Script executed:
#!/bin/bash # Searching for usage of notifySyncState in the codebase: rg -A 10 --fixed-strings 'notifySyncState'Length of output: 18511
Concurrency Synchronization Verification
After reviewing the call sites for
notifySyncStateChangedacross the repository, it appears that the chain manager already employs its own synchronization (e.g., via@synchronized(self)in DSChainManager). In thenotify_sync_state_caller()function, the lock onsyncInfoprotects the update and log calls, while subsequent notifications rely on DSChainManager’s internal thread safety.
- Recommendation:
• Verify that DSChainManager’s synchronization is robust enough when called concurrently, especially since multiple threads may callnotifySyncStateChanged.
• Consider implementing targeted concurrency tests or using thread sanitizer tools to ensure no race conditions manifest under heavy multithreaded usage.Overall, while the current locking strategy appears consistent with other parts of the codebase, please run dedicated concurrency tests to confirm that no edge-case race conditions exist.
1-16: No issues with license header.
79-89: Singleton implementation looks good.
This dispatch_once pattern is properly used here.
188-193:Details
✅ Verification successful
Confirm memory deallocation correctness.
Thedeallocmethod resets_coretoNULLafter callingdash_spv_apple_bindings_DashSPVCore_destroy, which is good. Ensure that no asynchronous or bridging code references_coreafter deallocation.
🏁 Script executed:
#!/bin/bash # Description: Search for references to `DSDashSharedCore._core` after `dealloc`. rg --context 3 "_core"Length of output: 16057
Memory deallocation verified and no asynchronous references detected.
Thergsearch confirms that_coreis referenced only in thedeallocmethod. No asynchronous or bridging code appears to access_coreafter it’s destroyed and set toNULL. The deallocation process—callingdash_spv_apple_bindings_DashSPVCore_destroy(_core)followed by_core = NULL—is correctly implemented.
- Verified that
_coreis only used locally withindealloc.- No external, asynchronous, or bridging references to
_corewere found.
297-317:Details
✅ Verification successful
Ensure thread safety for
notify_sync_state_caller.
The code synchronizes onsyncInfo, but it also referencescore.chain.chainManageroutside. Confirm that no concurrency conflicts arise from these calls.
🏁 Script executed:
#!/bin/bash # Description: Locate other references to `syncInfo` or `core.chain.chainManager` for concurrency usage patterns. rg -A 5 "syncInfo|chainManager"Length of output: 70563
Thread Safety in
notify_sync_state_callerVerifiedAfter reviewing the context, the usage of the
@synchronized(syncInfo)block to update the sync state and to invoke[core.chain.chainManager notifySyncStateChanged]is consistent with patterns found throughout the codebase. References tochainManagerin other parts of the project occur without additional locks, which indicates that its implementation (including thenotifySyncStateChangedmethod) is designed to be thread safe. No immediate concurrency concerns were observed, so no further changes appear necessary.DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.h (2)
24-28: Renamed methods align with revised domain language.
The rename from “blockchainIdentity*” to “identity*” clarifies the class purpose, improving consistency with “DSAssetLockDerivationPath.” Changes here look consistent with the rebranding.
24-28: Add unit tests for new derivation methods.
Confirm that each of these identity-related derivation path methods is tested to ensure correctness in address generation and funds derivation logic.DashSync/shared/Models/Derivation Paths/DSDerivationPath+Protected.h (1)
25-26: Make sure new methods have adequate test coverage
These new methods,walletBasedExtendedPublicKeyLocationStringForWalletUniqueID:andcreateIdentifierForDerivationPath, appear essential to your derivation path logic. Ensure test coverage for edge cases like empty or nil wallet unique IDs.DashSync/shared/Models/Derivation Paths/DSAssetLockDerivationPath.m (2)
41-90: Validate hardened indexes and references
In the chain-based methods (e.g.,identityRegistrationFundingDerivationPathForChain:), ensure the indexes and references align with your brand-new identity credit funding approach, especially regardingFEATURE_PURPOSE_IDENTITIES_SUBFEATURE_*constants. Double-check that these are recognized by the rest of the codebase.
97-99: Confirm recommended gap limit
The default gap limit is set to 5, which might be too small in certain scenarios. Verify that this is sufficient for real-world usage, especially where multiple transactions or addresses might be needed quickly.DashSync/shared/Models/Entities/DSMerkleBlockEntity+CoreDataClass.h (1)
43-47: Add error handling for entity creation methods
The newly introduced methods (merkleBlockEntityForBlockHash:...etc.) might fail under certain conditions (e.g., invalid block hash). Consider returningnilor handling errors so callers can respond appropriately.DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.m (2)
37-43: Confirm correct derivation path references
You have introducedbip32DerivationPathForAccountNumber:onChain:,bip44DerivationPathForAccountNumber:onChain:, andcoinJoinDerivationPathForAccountNumber:onChain:. Ensure each derivation path type (clear funds vs. anonymous funds) is properly recognized by the rest of the wallet logic, especially for transaction creation and identification.Also applies to: 46-54, 56-66
223-223: Validate new gap limit approaches
DSGapLimitFunds externalSingle,internalSingle, and custom gap limits (e.g., inreceiveAddressAtOffset:) may introduce unexpected address usage. Thoroughly test edge cases to ensure funds and addresses remain discoverable.Also applies to: 229-229, 240-240, 245-245, 251-251
DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.h (3)
8-8: Updated import for Dash SPV bindingsThe import has been changed from
dash_shared_core.htodash_spv_apple_bindings.h, which is consistent with the update to DashSharedCore 0.5.0 mentioned in the PR objectives.
22-23: Renamed class methods for better clarityThe methods have been renamed from
blockchainIdentitiesBLSKeysDerivationPathForWalletandblockchainIdentitiesECDSAKeysDerivationPathForWallettoidentitiesBLSKeysDerivationPathForWalletandidentitiesECDSAKeysDerivationPathForWalletrespectively. This simplifies the naming while maintaining functionality.
26-27: Return type change improves nullability handlingThe return types have been changed from
OpaqueKeytoDMaybeOpaqueKeyfor private key methods, which better indicates the possibility of a nil return value. This improves type safety and API clarity.Also applies to: 30-30
DashSync/shared/Models/Derivation Paths/DSGapLimit.m (4)
1-17: License header is correctly formattedThe license header is properly formatted and includes the correct copyright information and MIT license terms.
20-29: Well-implemented base DSGapLimit classThe DSGapLimit class follows good object creation patterns with factory methods that clearly express intent. The implementation is straightforward and maintainable.
31-64: Comprehensive DSGapLimitFunds implementationThe DSGapLimitFunds class provides various factory methods for different use cases (internal, external directions with configurable limits). The implementation follows consistent patterns and provides good flexibility.
67-79: DSGapLimitIdentity implementation handles identity-specific gap limitsThis class properly extends the base functionality to support identity-specific gap limits with optional identity ID. The implementation maintains consistency with the other gap limit classes.
DashSync/shared/Models/Entities/DSBlockchainIdentityEntity+CoreDataProperties.h (3)
10-10: Added import for new asset lock transaction entityThe import of
DSAssetLockTransactionEntity+CoreDataClass.his now required since the properties below use this type.
23-24: Updated property types for funding transactionsProperties have been changed to use
DSAssetLockTransactionEntityinstead of what was likelyDSCreditFundingTransactionEntitypreviously. This change reflects the architecture shift in how identities handle funding transactions in the new CoinJoin implementation.
40-43: Updated accessor methods to match new property typesThese accessor methods have been properly updated to use the new
DSAssetLockTransactionEntitytype, maintaining consistency with the property changes above.DashSync/shared/Categories/NSArray+Dash.h (2)
9-10: Added required imports for FFI bindingsThe imports for
dash_spv_apple_bindings.handDSKeyManager.hprovide the necessary type definitions for the new FFI-related categories below.
28-56: Added comprehensive FFI conversion utilities for arraysThese new categories provide bidirectional conversion between Objective-C arrays and various Rust-based types, including string vectors, 256-bit integer vectors, nested byte vectors, and block hash B-tree sets. These utilities are essential for the CoinJoin implementation that requires interoperability between Objective-C and Rust components.
Each category follows a consistent pattern with methods for:
- Converting from Rust to Objective-C (
ffi_from_*)- Converting from Objective-C to Rust (
ffi_to_*)- Proper memory management (
ffi_destroy_*)This approach ensures proper memory handling across language boundaries.
DashSync/shared/DashSync.xcdatamodeld/DashSync 22.xcdatamodel/contents (1)
21-30: Confirm compound index performance expectations
Compound indexes on DSAddressEntity (lines 21–30) and other entities can speed up complex queries at the cost of additional overhead on inserts and updates. Confirm through profiling that these indexes deliver benefits and do not negatively affect typical usage scenarios.DashSync/shared/Models/Chain/DSChain+Identity.h (2)
1-17: License header inclusion
No issues here. The MIT license header is properly included.
25-67: Consistent usage of out-parameters
The methods returningDSIdentityand optionally populating thefoundInWalletout-parameter are clearly documented. This pattern is consistent across the file.DashSync/shared/Models/Chain/DSChain+Identity.m (2)
31-37: Local identity count logic
MethodlocalIdentitiesCount(lines 31–37) accurately accumulates identity counts across available wallets. This looks correct and clear.
84-97: Foreign identity lookup
InidentityForUniqueId:foundInWallet:includeForeignIdentities:(lines 84–97), the logic falls back to a foreign identity lookup if not found in a local wallet. Ensure that foreign identities are indeed needed in this context, as it may cause unexpected references to identities beyond local wallet scope.DashSync/shared/Models/Chain/DSChain+Transaction.m (6)
18-41: Imports are relevant and well-scoped
They match usage in the file, with no extraneous references.
46-49: Method encapsulation for transaction access
This short method is an overload referencingtransactionForHash:returnWallet:. The approach is neat and helps keep the code DRY.
144-186: Partial registration check
When multiple wallets (owner, voting, operator, etc.) are discovered, partial registration is performed. Confirm if partial registration is expected or if the workflow should enforce a more uniform registration approach.
188-197: Registration logic
This cross-check for the corresponding provider registration transaction is consistent with the pattern inregisterProviderRegistrationTransaction, allowing partial registration if the matching wallet is found.
224-232: Logical consistency
This method registers a revocation transaction only if the corresponding provider registration is found. That approach is consistent and helps avoid orphaned transactions.
275-288: Centralized special transaction registration
This consolidates registration logic for different transaction types in one place, aiding maintainability. No immediate issues found.DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.h (2)
16-16: Method name clarity
indexOfKnownAddressHash:nicely parallelsindexOfKnownAddress:and expands functionality to direct hash lookups.
19-19: Return type changed from NSUInteger to uint32_t
Ensure this doesn't cause type mismatches if indexes can exceed 32 bits. If the derivation path length stays within that range, no issues.Are you expecting potential indexes beyond 32-bit?
DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.m (11)
8-17: New imports
All newly added imports (accounts, address entities, gap limit, etc.) appear relevant to the file's functionality.
21-39: Constructor readability
Splitting parameters across multiple lines aids clarity. The flow into the superclass initializer is straightforward.
44-49: Address loading sequence
After loading addresses from Core Data, the code setsaddressesLoadedand then applies default gap settings. The order is logical to ensure a consistent state.
61-70: Dynamic gap registration
Triggering gap registration upon discovering a used address keeps the derivation path synced. This is an effective approach for address coverage.
72-73: Extracting gap limit as a separate method
ProvidingdefaultGapSettingsclarifies the configuration and allows simpler adjustments in the future.
134-143: Retrieving first unused index
This method identifies the first index where no subsequent addresses have ever been used. The logic is straightforward and aligns with typical BIP44 patterns.
153-156: Index of known address hash
By converting the hash back to an address, you can leverage the existing indexing logic. This is a concise extension of the existing pattern.
168-177: Lazy address creation
Generating addresses on-demand respects cache usage and helps maintain performance. No issues are apparent here.
182-187: Private key lookup
This retrieves private keys for a specified range, referencingprivateKeyAtIndexPath:fromSeed:. The approach is direct and consistent.
189-213: Core Data loading
performBlockAndWaitensures thread-safe operations, and the validity check viaDIsValidDashAddressis a sensible safeguard.
215-228: Storing new addresses
Inserting addresses within aperformBlockhelps maintain Core Data consistency. This is a clean approach aligned with the rest of the class.DashSync/shared/Models/CoinJoin/Utils/DSMasternodeGroup.m (6)
39-52: Consider making_maxConnectionsan atomic or synchronized property.
Within this block,_maxConnectionsis declared asatomic, readonlybut is updated elsewhere (e.g.,updateMaxConnections) without explicit synchronization. In high-concurrency scenarios, readers and writers might access it simultaneously.Would you like a verification script to search for all unsynchronized usages of
_maxConnectionsto confirm safe access patterns?
134-156: Validate usage ofself.connectedPeersin concurrent contexts.
TheforPeer:port:warn:withPredicate:method copiesself.connectedPeersbefore iteration, which is good for concurrency. However, ensure that any modifications to peer sets within the loop happen after obtaining locks to avoid race conditions with other code paths mutating the sets in parallel.
509-565: Verify the extensive logic inpeer:disconnectedWithError:for consistency.
The method adjusts backoff times, moves peers between sets, and removes sessions frommasternodeMap. If an error or partial update occurs, a peer could remain in an inconsistent state. Confirm that all relevant data structures are always in sync.
1-16: Header & License Block – No Issues
No functional code to review here.
85-92: Consider atomic or locked access for_isRunning
_isRunningis set instartAsyncandstopAsyncbut read elsewhere (e.g., insidetriggerConnectionsJobWithDelay:on the dispatch queue). If_isRunningcan change while the queue is running, consider an atomic property or a lock around reads/writes to prevent any race conditions.
414-469: Validate masternode sessions before connecting
WithinconnectTo:, ensure thatpeeris truly associated with a valid session. If_masternodeMapor_sessionMapdata becomes stale or out-of-sync, it could lead to wasted connections or partial states. Consider extra checks or logging in casesessionIdis zero or missing.DashSync/shared/Models/Chain/DSChain.h (5)
64-65: Confirm whethermasternodeManagershould be a strong reference.
Using aweakproperty for a core manager can lead to unexpected deallocation if no other owners exist. Double-check thatDSChaindoesn’t outlive its manager in normal usage.
87-90: Ensure Core Data usage is thread-safe.
chainEntityInContext:andchainManagedObjectContextmay be accessed from multiple threads. Verify that the managed object context is used consistently, either confined to one queue or properly coordinated.
27-30: Contextual note on bridging
These added imports (dash_spv_apple_bindings.h,DSDashSharedCore.h,DSTransaction.h) strongly couple this header to the Rust bridging and shared core code. Ensure that these remain stable external dependencies and remain backward compatible over time.
64-65: PropertymasternodeManagerusage
Currently declared asweak. Verify that the referenced manager outlivesDSChain. If not, references to a deallocated manager can lead to unexpected behavior.
234-235: Confirm correctness of new Bloom filter addresses
- (NSArray<NSString *> *)newAddressesForBloomFilter;is newly introduced. Ensure adequate test coverage so that newly generated addresses align with the chain’s derivation paths and remain valid in all operational contexts.DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (4)
56-137: Prevent possible race conditions inregisterCoinJoin:.
This method initializes_clientManagerunder@synchronized (self). IfregisterCoinJoin:is called repeatedly or concurrently, ensure that_clientManageris not reinitialized or left in a partial state.
608-652: Validate session lifecycle transitions insessionLifecycleListenerto avoid partial states.
The method handles two different states (session completed vs. started). Check if any intermediate states or error conditions get lost, leaving the session data in a partially updated state.
146-153: Check for merging new options gracefully
dash_spv_coinjoin_coinjoin_client_manager_CoinJoinClientManager_change_options(self.clientManager, options)overwrites existing client manager settings. If partial updates are needed, ensure logic merges old and new values or logs the difference.
464-512: Nested block synchronization caution
IncommitTransaction, the code runs a completion blockonPublished:^(UInt256 txId, NSError * _Nullable error) { ... }and re-enters@synchronized(context). While the approach is valid, be mindful not to recursively acquire the same lock from within the block if the block is invoked synchronously in certain error paths.DashSync/shared/Models/Chain/DSChain+Params.h (1)
25-145: Commend thorough documentation, verify pointer usage.
Most properties here have excellent doc comments. Ensure pointer properties likechainTypeuseassignintentionally (e.g., for raw C structures). If it is an Objective-C object, preferstrongorweak.DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.m (1)
105-118: Validate the indexing logic and key derivation.
TheinitWithIndexes:...method sets up indexes without verifying their range. For user safety, consider bounds checking or input validation to avoid unexpected behavior ifindexes[]contains invalid data.DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.h (2)
33-36: Note parameter type change for extendedPublicKey.The type for
extendedPublicKeyparameter has changed fromOpaqueKeytoDMaybeOpaqueKey *. Ensure that all code passing this parameter is updated to use the correct type, as this is a breaking change to the method signature.
24-25:Details
❓ Verification inconclusive
Breaking API change: Property and method renames require client code updates.
This file contains significant API changes:
- Renamed properties from
contactSourceBlockchainIdentityUniqueIdtocontactSourceIdentityUniqueId- Changed method signatures to remove "blockchain" from identity references
- Changed parameter types (e.g.,
forAccountNumbertoforAccountwith typeDSAccount *)These changes will require updates to all code that uses these properties and methods. Ensure that all calling code is updated to reflect these changes, or consider providing compatibility methods during the transition period.
Also applies to: 27-42
🏁 Script executed:
#!/bin/bash # Find references to the old property and method names that need to be updated echo "Searching for old property and method references that need updating..." # Search for old property names echo "References to old property names:" rg "contactSourceBlockchainIdentityUniqueId|contactDestinationBlockchainIdentityUniqueId" --type objc # Search for old method names with 'blockchainIdentity' echo "References to old method names with blockchainIdentity:" rg "contactBasedDerivationPathWithDestinationBlockchainIdentityUniqueId|withDestinationBlockchainIdentityUniqueId|sourceBlockchainIdentityUniqueId" --type objcLength of output: 614
API Changes Verified – Please Manually Confirm Client Updates
The renaming in DSIncomingFundsDerivationPath (e.g., changing from
contactSourceBlockchainIdentityUniqueIdtocontactSourceIdentityUniqueId, updating method signatures, and modifying parameter types) seems to have been applied consistently. Our automated search did not produce any references to the old names, which is a positive sign.However, given that no output was returned by the search, please manually verify that all client references to these properties and methods have been updated accordingly, or that appropriate compatibility methods are in place during the transition.
- Affected File:
DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.h(notably lines 24–25 and 27–42)- Action Required: Manually confirm that the calling code no longer uses the old API names and method signatures.
DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.m (1)
50-55:Details
❓ Verification inconclusive
Address TODO comment about identityID.
There's a TODO comment indicating that identityID may always be 0. This should be investigated and either confirmed as intended behavior or fixed. If it's intentional, the comment should be replaced with an explanation.
🌐 Web query:
What is the purpose of identityID in Dash blockchain?💡 Result:
The IdentityID in the Dash blockchain serves as a unique identifier for decentralized identities (DIDs) within Dash Platform, enabling secure, user-controlled interactions while maintaining privacy and tamper resistance. Here’s a detailed breakdown:
Core Purpose
Unique Identity Verification
IdentityID acts as a cryptographically generated identifier that distinguishes one identity from others on the network[3][14]. It ensures no two identities can be duplicated, addressing issues like impersonation or "double-spending" of identities[2].Decentralized Identity Ownership
Users create and own their identities, which are anchored to the blockchain. The IdentityID enables:
- Self-sovereign control: Users manage their data without relying on third parties[9][14].
- Tamper-proof records: Immutable blockchain storage prevents unauthorized alterations[2][14].
Facilitating dApp Interactions
IdentityID replaces public key hashes as a primary identifier in decentralized applications (dApps), simplifying user interactions (e.g., payments, data storage) while retaining privacy[2][12]. For example:IdentityID → "user123.dash" (via Dash Platform Name Service)This replaces complex wallet addresses with human-readable identifiers[12].
Technical Implementation
- Credit System Integration:
Each identity linked to an IdentityID holds a balance of "credits" (backed by locked DASH tokens), which pay for network operations like data storage or transactions[2][3].- Governance and Security:
Dash’s masternode network secures identities using X11 hashing and ChainLocks, preventing 51% attacks or tampering[2][11].Comparison to Traditional Systems
Feature Traditional Identity Systems Dash IdentityID Control Centralized entities (e.g., governments) User-owned, self-managed[14] Security Vulnerable to breaches Blockchain-backed, tamper-resistant[2][3] Privacy Data exposed to intermediaries PrivateSend/CoinJoin integration[4][19] Use Cases
- Financial Inclusion: Enables unbanked users to create immutable economic identities[14].
- KYC Compliance: Supports privacy-preserving verification for decentralized finance (DeFi) protocols[6][9].
By anchoring identities to its blockchain, Dash ensures users retain control while enhancing security and usability—a critical step toward decentralized trust ecosystems[2][9].
Citations:
- 1: https://weareblox.com/en-eu/dash
- 2: https://www.dash.org/blog/creating-identity-on-dash-platform/
- 3: https://platform-explorer.com/identities
- 4: https://support.binance.us/en/articles/9843899-all-about-dash
- 5: https://en.wikipedia.org/wiki/Dash_(cryptocurrency)
- 6: https://hedera.com/blog/the-rise-of-the-identity-token
- 7: https://blokt.com/guides/what-is-dash
- 8: https://news.asu.edu/20180118-solutions-navigate-your-way-through-dash-bitcoin-and-smart-contracts
- 9: https://hedera.com/learning/data/decentralized-identity
- 10: https://www.finextra.com/blogposting/25698/dash-dashx-navigating-the-currents-of-cryptocurrency-markets--an-analytical-deep-dive-into-tren
- 11: https://flyp.me/en/top-cryptocurrencies/what-is-dash/
- 12: https://iq.wiki/wiki/dash
- 13: https://www.markets.com/education-centre/is-dash-a-good-cryptocurrency-for-traders/
- 14: https://dashdevs.com/blog/how-to-unlock-the-future-of-financial-services-with-decentralized-identity-did/
- 15: https://www.thebigwhale.io/tokens/dash
- 16: https://www.tradefinanceglobal.com/blockchain/dash/
- 17: https://www.exodus.com/assets/docs/dash-whitepaper.pdf
- 18: https://github.com/dashpay/dash/wiki/whitepaper
- 19: https://academy.youngplatform.com/en/cryptocurrencies/dash-monero-zcash-privacy-coins/
- 20: https://www.avatrade.com/trading-info/financial-instruments-index/cryptocurrencies/dash
ACTION REQUIRED: Verify and Document the Use of identityID
The current implementation in
DSAuthenticationKeysDerivationPath.malways usesidentityID:0for non-single user authentication paths. Given that in the Dash blockchain the identityID must serve as a unique identifier for decentralized identities, using a hardcoded 0 can be misleading. Please verify whether using 0 here is intentional (e.g., as a placeholder when no valid identity exists) or if it’s a mistake. If it is intended, update the TODO comment with a clear explanation of why a zero value is appropriate; otherwise, adjust the implementation to provide a proper unique identifier.
- File:
DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.m- Lines: 50–55
- Snippet Reference:
- (DSGapLimit *)defaultGapSettings { // TODO: identityID always 0?? return self.type == DSDerivationPathType_SingleUserAuthentication ? [super defaultGapSettings] : [DSGapLimitIdentity withLimit:10 identityID:0]; }DashSync/shared/Models/Derivation Paths/DSDerivationPathFactory.h (1)
26-30:Details
✅ Verification successful
Breaking API change: Renamed methods and class types.
The following changes will require client code updates:
- Methods renamed from
blockchainIdentity*toidentity*- Return type changed from
DSCreditFundingDerivationPathtoDSAssetLockDerivationPathThese changes align with the renaming pattern across the codebase but will require updates to all calling code.
🏁 Script executed:
#!/bin/bash # Find references to the old method names and class types echo "Searching for old method and class references that need updating..." # Search for DSCreditFundingDerivationPath references echo "References to DSCreditFundingDerivationPath:" rg "DSCreditFundingDerivationPath" --type objc # Search for old method names with blockchainIdentity echo "References to old method names with blockchainIdentity:" rg "identityRegistrationFundingDerivationPathForWallet|identityTopupFundingDerivationPathForWallet|identityInvitationFundingDerivationPathForWallet|blockchainIdentityBLSKeysDerivationPathForWallet|blockchainIdentityECDSAKeysDerivationPathForWallet" --type objcLength of output: 8018
Critical Update: Breaking API Changes Verified
The changes in
DSDerivationPathFactory.hhave been successfully propagated. Our investigation confirms:
- Method Renaming: All method calls formerly using the
blockchainIdentity*prefix have been updated to use theidentity*naming convention.- Return Type Change: References to the old
DSCreditFundingDerivationPathno longer exist; all methods now correctly returnDSAssetLockDerivationPath.- Client Impact: While the internal code and tests (e.g., in
Example/Tests/DSDIP14Tests.mand related files) have been updated, note that any external client code relying on the previous API will need corresponding updates.No additional issues were found in the codebase, and the breaking changes are properly implemented.
DashSync/shared/Models/CoinJoin/DSCoinJoinManager.h (2)
61-61: Retain versus assign for Objective-C object pointer.
This property is still declared withassign; however, storing an Objective-C object viaassigncan lead to crashes if it’s deallocated elsewhere. Usingstrongorweakis typically recommended.
Since a similar comment was mentioned in prior reviews, marking as duplicate.-@property (nonatomic, assign, nullable) DSChain *chain; +@property (nonatomic, strong, nullable) DSChain *chain;
63-63: Confirm whetherDCoinJoinClientOptions *can safely beassign.
IfDCoinJoinClientOptionsis a Core Foundation/struct-based pointer,assignmight be correct. But if it’s an Objective-C object, consider usingstrongorweak. Check for potential reference-counting issues.DashSync/shared/Models/Derivation Paths/DSDerivationPathFactory.m (16)
9-19: Imports look appropriate and consistent.
These newly added imports bring in references to related classes and categories needed for identity and asset lock derivation. No immediate concerns.
33-37: Thread-safety consideration for newly added identity path dictionaries.
Each dictionary is mutable and shared. You do use@synchronized(self)in subsequent methods to guard usage, which may suffice, but re-check for any concurrency issues if these properties are also accessed beyond these methods.
150-169: Identity registration derivation path logic appears consistent.
Code sets up a lazy initialization pattern usingdispatch_onceand@synchronized(self). No immediate issues. Looks good.
171-188: Top-up derivation path uses the same pattern as registration.
Implementation mirrors the registration path approach, ensuring consistent lazy initialization. Good for maintainability.
190-207: Invitation funding derivation path follows the same idiom.
No major comments; the code flow is consistent and easy to follow.
209-229: BLS key derivation path addition.
Implementation ensures addresses are preloaded if the extended private key or non-hardened extended public key is present. This is fine. Confirm that loading addresses is also performed in all necessary scenarios.
230-248: ECDSA key derivation path uses a parameter assertion.
NSParameterAssert(wallet)is used. This is acceptable, but confirm that upstream code gracefully handles or logs errors if the wallet is missing. Otherwise, logic is consistent with the BLS derivation path.
258-262: Integration of the new identity paths in loaded specialized derivation paths.
All new calls are guarded byif (wallet.chain.isEvolutionEnabled). Looks straightforward. Ensure you have coverage in tests for these paths.
334-355: Unloaded specialized paths now include new identity derivation paths.
No immediate concerns. The factoring out of each path in a single method is readable.
360-414: Private key extraction via ffi calls.
Memory handling appears correct: script usesslice_ctorand releases or destroys allocations. Check for any potential memory leaks in your bridging code, though it seems well-managed with your patterns.
416-455: Extended public key serialization/deserialization.
Implementation is well-structured, with separate methods for partial and full arguments. Confirm that all error scenarios (invalid string, mismatched chain) are handled as expected.
459-461: Secondary helper for extended public key deserialization.
No functional issues. Null return on failure is consistent with the primary method.
465-467: Standalone public key location.
Naming is explicit, which is good. No concerns.
469-471: Standalone info dictionary string.
Again, descriptive naming. Straightforward usage.
473-475: Wallet-based extended public key location.
Same pattern as above, no specific issues.
477-479: Wallet-based extended private key location.
Consistent approach for private key references. Looks good.DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (31)
1-16: License header addition.
No concerns; license is properly stated.
19-40: Imports cover a wide range of new classes for mixing functionality.
Integration looks aligned with the new CoinJoin approach. No immediate issues.
41-44: Constants for block depth and default parameters.
Names are clear. The defaults (e.g.,DEFAULT_MAX_DEPTH = 9999999) appear arbitrary but acceptable for the environment.
45-55: Private properties for concurrency and caching.
Usingatomicon numeric properties is helpful. Keep in mind thatatomicon Obj-C objects isn’t always a complete concurrency fix; the queue-based approach should suffice.
59-78: Singleton approach.
Storing managers in_managerChainDictionaryis fine. Watch for potential memory growth if many chain objects are created and not freed, but likely not an issue in normal usage.
80-93: Init method with a newly added wrapper instance.
Invokes[self printUsedKeys]at the end. This is safe, but confirm that the logs or results won’t cause a race if the chain manager is not yet fully constructed.
95-97: Initializing masternode group.
Straightforward usage. Make sure the group is started or stopped consistently elsewhere.
99-103: Client options creation with defaults.
CoinJoin parameters are properly encapsulated. No issues found.
134-147: configureMixingWithAmount:... sets up multi session and denom goals.
All fields are updated in self.options, with a subsequent wrapper call if registered. No concerns.
222-280: Stop logic, shutdown, and transaction receive notifications.
The flow for shutting down mixing and halting the timer is well-organized. The code gracefully unsubscribes from notifications.
282-315: autoDenominating routine with success/failure flags.
Approach re-uses the wrapper’s registration method. Good logging on success/failure. No immediate issues.
326-329: setStopOnNothingToDo: calls wrapper logic.
Adheres to the user’s preference for continuing or stopping. Straightforward bridging code.
334-349: Reading and processing peer messages for coinjoin.
The checks forself.isMixingandself.isShuttingDownbefore calling wrapper are correct. No concerns.
350-363: isMineInput check.
Method quickly finds related address in the correct account. Clean approach.
365-486: selectCoinsGroupedByAddresses logic.
Iterating unspent outputs and skipping certain amounts/demoms is thorough. The concurrency block is properly used. Just ensure large UTXO sets don’t cause performance issues.
648-700: getMixingProgress method.
Calculates average rounds across inputs, logs progress. This is well-coded. Keep an eye on performance if large sets of outputs are enumerated frequently.
702-733: Aggregation of outputs by denominations.
Implementation is straightforward. The code first sets up categories, then iterates over unspent outputs. Approve.
735-745: isCoinJoinOutput check.
Method returns quickly if the output is not denominated or not fully mixed. Clear logic.
747-785: getBalance method.
Accumulates anonymized and denominated amounts by scanning outputs. Straightforward. No issues.
815-846: Address generation logic (freshAddress + getIssuedReceiveAddresses + getUsedReceiveAddresses).
Implementation references coinJoin derivation paths. Straightforward bridging to DSAccount. No concerns.
848-871: commitTransactionForAmounts.
Checks for transaction sign and publishes. Logging is done with references to “” outside debug mode, which is good for privacy.
873-879: Fetching a masternode entry by proTxHash.
Bridges to the shared processor. Straightforward one-liner. No concerns.
881-891: disconnectMasternode.
Method defers actual logic tomasternodeGroup. All good.
893-911: sendMessageOfType with given IP.
Dispatch-based usage to find the correct peer. The approach is robust for modular message sending.
913-931: Handling success block and wait logic.
Update success block height, then check if we must wait for a new block under single-session mixing. Straightforward gating mechanism.
933-935: coinJoinTxTypeForTransaction
Exposes a wrapper method to differentiate transaction type. Clear approach.
937-942: calculateAnonymizableBalanceWithSkipDenominated.
Asynchronous wrapper call. No issues apparent, callback usage is standard.
944-955: minimumAnonymizableBalanceWithCompletion.
Checks collateral if it exists, returning the smallest denom plus overhead. Logic is concise.
957-959: getSmallestDenomination.
Simple pass-through to the wrapper. Straightforward.
961-982: selectCoinJoinUTXOs.
Filters out locked or non-denominated outputs to build a coin control set. Implementation is consistent with earlier logic.
993-1075: Session event callbacks.
Handles session start/complete, mixing events, and transaction-processed notifications. Logging and delegation usage appear correct. No concerns.DashSync/shared/Models/Chain/DSChain+Params.m (37)
1-16: License header.
The MIT license block is standard and correct. No issues.
18-39: New includes and metadata defines.
These macro definitions for protocol version, spork keys, and contract IDs are consistent. Approved.
41-44: Strings for spork keys and chain param references.
Clear naming. Good approach for storing keys with descriptive constants.
66-72: ChainType getter/setter.
Immediately returns or sets pointer values via NSValue. Fine for bridging to rust-based chain type definitions.
80-84: setProtocolVersion for devnet.
Only storing protocol version in the keychain for devnet. That usage is consistent with the code logic for main/test net defaults.
86-101: protocolVersion logic for the three chain types.
Retrieves from the keychain for devnet, else uses pre-compiled constants. This is appropriate for dynamic devnet usage.
103-127: isRotatedQuorumsPresented approach.
Caches the boolean in runtime. Takes the stored dev/test/main net values from keychain. The flow is consistent. No issues.
129-145: Setter for rotated quorums.
Again, dev/test/main net usage matches the pattern. Approved.
146-174: minProtocolVersion logic with synchronized block.
Uses keychain fetch, merges with default min protocol version, then caches. All good.
176-196: setMinProtocolVersion with range checks.
Ensures it does not exceed valid bounds. Matches other usage. Approved.
198-219: standardPort getters/setters.
Same devnet logic for storing in keychain or returning default for main/test net. No issues.
228-255: standardDapiGRPCPort getters/setters.
Same generalized approach for devnet usage. Straightforward. Approved.
257-283: standardDapiJRPCPort getters/setters.
The pattern is consistent with gRPC. No concerns.
287-307: maxProofOfWork logic.
Checks cached object or defaults to chain-based. Good usage ofobjc_setAssociatedObject.
309-315: allowMinDifficultyBlocks.
Direct pass-through to the rust-based config. Approved.
317-323: Base reward & coinType.
Appropriate logic for test vs main net. Good defaults.
327-350: Spork key retrieval from keychain for devnet.
Storing spork keys only for devnet. This matches the rest of the devnet approach. Approved.
369-386: Spork address logic.
Again, retrieving from keychain for devnet. The pattern is consistent.
396-417: platformProtocolVersion logic.
Same pattern for devnet, with fallback to main/test net constants. Looks good.
419-464: dpnsContractID (get/set).
Stores data in the keychain for devnet, returns known defaults or zero for other chain types. Straightforward.
466-511: dashpayContractID (get/set).
Same pattern as DPNS. No immediate issues.
513-537: Minimum difficulty blocks for devnet.
Aligned with the rest. Zero for non-devnet. Approve.
541-548: uniqueID logic based on shortHexString of genesisHash.
Nice approach. Approved.
550-564: networkName logic.
Returns “main” or “test,” or defers to a dev net. Straightforward.
566-581: localizedName for UI.
Provides human-readable names for each chain type. No issues.
585-589: setDevnetNetworkName stub.
Currently sets the name to “Evonet” if it’s devnet. Hard-coded but presumably correct for your environment.
591-593: transactionVersion.
Just returns chain-based default. Fine.
595-597: peerMisbehavingThreshold.
Relies on rust bridging. Straightforward.
600-607: headersMaxAmount property.
Stored with associated object. No concurrency concerns if set from a single thread. Approved.
608-616: genesisHash property.
Store/retrieve using associated objects. Straightforward.
618-621: minOutputAmount dynamic calculation.
Combines standard TX minimum with chain’sfeePerByte. Logic is correct to ensure it never goes below TX_MIN_OUTPUT_AMOUNT.
627-635: feeForTxSize.
Provides standard fee vs. dynamic fee approach. The fallback logic is fine. No concerns.
637-643: feePerByte bridging.
Simple associated object usage. Matches the approach used throughout for devnet updates.
647-657: Chain type checks.
isMainnet,isTestnet,isDevnetAnyrely on rust bridging. No concerns.
659-661: isEvolutionEnabled always returns YES.
Indicates the new identity features are active for all net types. Approved for your dev usage unless you intend to toggle it eventually.
663-665: isDevnetWithGenesisHash.
Compares the stored genesis hash. Clean logic.
669-679: core19/core20 activation checks.
Queries chain’s lastTerminalBlockHeight and compares with activation heights from rust bridging. Straightforward.DashSync/shared/DashSync.h (2)
9-40: New imports and definitions look consistent.
Imports related to identity, chain, and CoinJoin are aligned with the refactoring goal and appear properly scoped to this header.Also applies to: 67-68, 115-115
9-9:Details
✅ Verification successful
Validate newly introduced imports and references.
Multiple identity and CoinJoin-related imports were added (e.g.,"DSIdentity.h","DSCoinJoinManager.h", and categories like"DSChain+Identity.h"). Ensure all references are necessary and actively used in the codebase to avoid bloating the header.Also applies to: 12-17, 19-19, 24-25, 35-40, 67-68, 115-115
🏁 Script executed:
#!/bin/bash # Description: Check usage references of newly added imports and categories. rg --context 3 "DSCoinJoinManager|DSChain\+Identity|DSAssetLockDerivationPath"Length of output: 63517
Validation Complete: Newly Introduced Imports Are Actively Used
After reviewing the usage across the codebase, the identity and CoinJoin-related imports (including
"DSIdentity.h","DSCoinJoinManager.h","DSChain+Identity.h", and"DSAssetLockDerivationPath.h") are confirmed to be in active use in multiple modules. For example:
- DSCoinJoinManager: Utilized in network handling, CoinJoin processing in files like
DSPeer.m,DSCoinJoinManager.m, and various CoinJoin wrapper and view controller classes.- DSChain+Identity: Referenced in chain management, identity setup, and wallet modules.
- DSAssetLockDerivationPath: Employed extensively in derivation path factory methods, transaction processing (e.g., in
DSAssetLockTransaction.m), and identity funding procedures.The verification through our search confirms that these additions are necessary and not redundant. No excessive bloat has been introduced, so no changes to these imports are required.
| return [NSError errorWithCode:0 descriptionKey:DSLocalizedFormat(@"Wrong key length", nil, ffi_ref->wrong_length)]; | ||
| case dash_spv_crypto_keys_KeyError_Extended: | ||
| return [NSError errorWithCode:0 descriptionKey:DSLocalizedFormat(@"Key extended error", nil, ffi_ref->extended)]; | ||
| case dash_spv_crypto_keys_KeyError_UnableToDerive: | ||
| return [NSError errorWithCode:0 localizedDescriptionKey:@"Unable to derive key"]; | ||
| case dash_spv_crypto_keys_KeyError_DHKeyExchange: | ||
| return [NSError errorWithCode:0 localizedDescriptionKey:@"Unable to exchange key"]; | ||
| case dash_spv_crypto_keys_KeyError_CCCrypt: | ||
| return [NSError errorWithCode:0 descriptionKey:DSLocalizedFormat(@"CCrypt error", nil, ffi_ref->cc_crypt)]; | ||
| case dash_spv_crypto_keys_KeyError_EmptySecKey: | ||
| return [NSError errorWithCode:0 localizedDescriptionKey:@"Private key is empty"]; | ||
| case dash_spv_crypto_keys_KeyError_Product: | ||
| return [NSError errorWithCode:0 localizedDescriptionKey:@"Can't multiple keys"]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include placeholders in string formatting and fix grammatical text.
Some lines appear to use DSLocalizedFormat(@"some text", nil, someValue) without a placeholder in the string, causing the inserted values to be dropped or ignored (e.g., lines 48, 50, 56). Also, the message “Can't multiple keys” (line 60) seems grammatically incorrect; consider “Can't multiply keys” or another clearer phrase.
| #define uint768_hex(u) [NSData dataWithUInt768:u].hexString | ||
| #define uint768_reverse_hex(u) [NSData dataWithUInt768:u].reverse.hexString | ||
| #define ecpoint_hex(u) [NSData dataWithBytes:u.p length:sizeof(DSECPoint)].hexString | ||
| #define dsutxo_hex(u) [NSData dataWithBytes:u.p length:sizeof(DSUTXO)].hexString |
There was a problem hiding this comment.
Review implementation of dsutxo_hex macro
The new macro dsutxo_hex appears to have an implementation issue. It attempts to access a field p in DSUTXO struct (via u.p), but looking at the DSUTXO struct definition (lines 79-82), there is no field named p - it only contains hash and n fields.
The correct implementation should likely be:
-#define dsutxo_hex(u) [NSData dataWithBytes:u.p length:sizeof(DSUTXO)].hexString
+#define dsutxo_hex(u) [NSData dataWithBytes:&u length:sizeof(DSUTXO)].hexString📝 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.
| #define dsutxo_hex(u) [NSData dataWithBytes:u.p length:sizeof(DSUTXO)].hexString | |
| #define dsutxo_hex(u) [NSData dataWithBytes:&u length:sizeof(DSUTXO)].hexString |
| - (void)signWithKey:(DOpaqueKey *)key { | ||
| NSParameterAssert(key); | ||
| // ECDSA | ||
| self.signature = [DSKeyManager NSDataFrom:key_ecdsa_sign(key->ecdsa, self.governanceVoteHash.u8, 32)]; | ||
| Slice_u8 *slice = slice_u256_ctor_u(self.governanceVoteHash); | ||
| Vec_u8 *result = DECDSAKeySign(key->ecdsa, slice); | ||
| self.signature = [DSKeyManager NSDataFrom:result]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for ECDSA signing.
Currently, the code does not check whether DECDSAKeySign fails or returns an empty signature. Consider adding safety checks to handle potential failures.
- (void)signWithKey:(DOpaqueKey *)key {
NSParameterAssert(key);
// ECDSA
Slice_u8 *slice = slice_u256_ctor_u(self.governanceVoteHash);
Vec_u8 *result = DECDSAKeySign(key->ecdsa, slice);
self.signature = [DSKeyManager NSDataFrom:result];
+ if (!self.signature || [self.signature length] == 0) {
+ // Handle error, e.g., log a warning or set an error state
+ }
}📝 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.
| - (void)signWithKey:(DOpaqueKey *)key { | |
| NSParameterAssert(key); | |
| // ECDSA | |
| self.signature = [DSKeyManager NSDataFrom:key_ecdsa_sign(key->ecdsa, self.governanceVoteHash.u8, 32)]; | |
| Slice_u8 *slice = slice_u256_ctor_u(self.governanceVoteHash); | |
| Vec_u8 *result = DECDSAKeySign(key->ecdsa, slice); | |
| self.signature = [DSKeyManager NSDataFrom:result]; | |
| } | |
| - (void)signWithKey:(DOpaqueKey *)key { | |
| NSParameterAssert(key); | |
| // ECDSA | |
| Slice_u8 *slice = slice_u256_ctor_u(self.governanceVoteHash); | |
| Vec_u8 *result = DECDSAKeySign(key->ecdsa, slice); | |
| self.signature = [DSKeyManager NSDataFrom:result]; | |
| if (!self.signature || [self.signature length] == 0) { | |
| // Handle error, e.g., log a warning or set an error state | |
| } | |
| } |
| + (Vec_u32 *)ffi_to:(NSIndexPath *)obj { | ||
| NSUInteger length = obj.length; | ||
| uint32_t *indexes = malloc(sizeof(uint32_t) * length); | ||
| for (NSUInteger i = 0; i < length; i++) { | ||
| indexes[i] = (uint32_t)[obj indexAtPosition:i]; | ||
| } | ||
| free(entry); | ||
| return Vec_u32_ctor(length, indexes); | ||
|
|
||
| } |
There was a problem hiding this comment.
Add memory allocation validation.
The ffi_to method allocates memory using malloc but doesn't check if the allocation was successful. This could lead to crashes if memory allocation fails.
Add a check after memory allocation:
+ (Vec_u32 *)ffi_to:(NSIndexPath *)obj {
NSUInteger length = obj.length;
uint32_t *indexes = malloc(sizeof(uint32_t) * length);
+ if (!indexes && length > 0) {
+ NSLog(@"Failed to allocate memory for indexes array");
+ return NULL;
+ }
for (NSUInteger i = 0; i < length; i++) {
indexes[i] = (uint32_t)[obj indexAtPosition:i];
}
return Vec_u32_ctor(length, indexes);
}📝 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.
| + (Vec_u32 *)ffi_to:(NSIndexPath *)obj { | |
| NSUInteger length = obj.length; | |
| uint32_t *indexes = malloc(sizeof(uint32_t) * length); | |
| for (NSUInteger i = 0; i < length; i++) { | |
| indexes[i] = (uint32_t)[obj indexAtPosition:i]; | |
| } | |
| free(entry); | |
| return Vec_u32_ctor(length, indexes); | |
| } | |
| + (Vec_u32 *)ffi_to:(NSIndexPath *)obj { | |
| NSUInteger length = obj.length; | |
| uint32_t *indexes = malloc(sizeof(uint32_t) * length); | |
| if (!indexes && length > 0) { | |
| NSLog(@"Failed to allocate memory for indexes array"); | |
| return NULL; | |
| } | |
| for (NSUInteger i = 0; i < length; i++) { | |
| indexes[i] = (uint32_t)[obj indexAtPosition:i]; | |
| } | |
| return Vec_u32_ctor(length, indexes); | |
| } |
| - (DSBlock *_Nullable)blockUntilGetInsightForBlockHeight:(uint32_t)blockHeight; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New blocking method for retrieving blocks by height.
This method complements the existing method for retrieving blocks by hash. Consider adding documentation that clarifies this is a blocking call, which could impact UI responsiveness if called from the main thread.
🏁 Script executed:
#!/bin/bash
# Check if this method is called from any UI/main thread contexts
rg -A 3 -B 3 "blockUntilGetInsightForBlockHeight:" --glob "*.m"Length of output: 1406
Main-Thread Safety: Add Documentation on Blocking Behavior
We verified that the blockUntilGetInsightForBlockHeight: method indeed uses a semaphore (as seen in DSChain.m), and it's invoked in contexts such as DSDashSharedCore.m. Although there's no direct evidence of its usage on the main thread, its blocking nature warrants an explicit documentation note in DashSync/shared/Models/Chain/DSChain+Protected.h to caution developers about potential UI responsiveness issues if used on the main thread.
- Action: Add a documentation comment to the method declaration in
DashSync/shared/Models/Chain/DSChain+Protected.hclearly stating that it is a blocking call and should not be invoked from the main thread.
| + (instancetype)contactBasedDerivationPathWithDestinationIdentityUniqueId:(UInt256)destinationIdentityUniqueId | ||
| sourceIdentityUniqueId:(UInt256)sourceIdentityUniqueId | ||
| forAccount:(DSAccount *)account | ||
| onChain:(DSChain *)chain { | ||
| NSAssert(!uint256_eq(sourceIdentityUniqueId, destinationIdentityUniqueId), @"source and destination must be different"); | ||
| UInt256 indexes[] = {uint256_from_long(FEATURE_PURPOSE), uint256_from_long((uint64_t) chain.coinType), uint256_from_long(FEATURE_PURPOSE_DASHPAY), uint256_from_long(account.accountNumber), sourceIdentityUniqueId, destinationIdentityUniqueId}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure clearer error handling for identical source/destination IDs.
Currently, there's an NSAssert(!uint256_eq(...)) check. If triggered in production, it would crash. Consider returning nil or providing an error object to gracefully handle this case at runtime.
| - (void)storeNewAddressInContext:(NSString *)address | ||
| atIndex:(uint32_t)n | ||
| identityIndex:(uint32_t)identityIndex | ||
| context:(NSManagedObjectContext *)context { | ||
| [self.managedObjectContext performBlock:^{ // store new address in core data | ||
| DSDerivationPathEntity *derivationPathEntity = [DSDerivationPathEntity derivationPathEntityMatchingDerivationPath:self inContext:self.managedObjectContext]; | ||
| DSAddressEntity *e = [DSAddressEntity managedObjectInContext:self.managedObjectContext]; | ||
| e.derivationPath = derivationPathEntity; | ||
| NSAssert(DIsValidDashAddress(DChar(address), self.chain.chainType), @"the address is being saved to the wrong derivation path"); | ||
| e.address = address; | ||
| e.index = n; | ||
| e.identityIndex = identityIndex; | ||
| e.standalone = NO; | ||
| }]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential retain cycle in block capture.
The method storeNewAddressInContext:atIndex:identityIndex:context: uses a block that captures self:
[self.managedObjectContext performBlock:^{
DSDerivationPathEntity *derivationPathEntity = [DSDerivationPathEntity derivationPathEntityMatchingDerivationPath:self inContext:self.managedObjectContext];
// ...
}];This could lead to a retain cycle if the block is stored by the managed object context. Consider using a weak reference to self:
__weak typeof(self) weakSelf = self;
[self.managedObjectContext performBlock:^{
__strong typeof(weakSelf) strongSelf = weakSelf;
if (!strongSelf) return;
// Use strongSelf instead of self
}];| + (DMaybeOpaqueKeys *)privateKeysAtIndexPaths:(NSArray *)indexPaths | ||
| fromSeed:(NSData *)seed | ||
| derivationPath:(DSDerivationPath *)derivationPath; | ||
| + (NSArray<NSString *> *)serializedPrivateKeysAtIndexPaths:(NSArray *)indexPaths | ||
| fromSeed:(NSData *)seed | ||
| derivationPath:(DSDerivationPath *)derivationPath; | ||
| + (NSString *_Nullable)serializedExtendedPublicKey:(DSDerivationPath *)derivationPath; | ||
|
|
||
| + (NSString *)serializedExtendedPrivateKeyFromSeed:(NSData *)seed | ||
| derivationPath:(DSDerivationPath *)derivationPath; | ||
| + (NSData *)deserializedExtendedPublicKey:(NSString *)extendedPublicKeyString onChain:(DSChain *)chain; | ||
| + (NSData *)deserializedExtendedPublicKey:(NSString *)extendedPublicKeyString | ||
| onChain:(DSChain *)chain | ||
| rDepth:(uint8_t *)depth | ||
| rTerminalHardened:(BOOL *)terminalHardened | ||
| rTerminalIndex:(UInt256 *)terminalIndex; | ||
| + (NSData *)deserializedExtendedPublicKey:(DSDerivationPath *)derivationPath extendedPublicKeyString:(NSString *)extendedPublicKeyString; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document new key handling methods.
Multiple new methods for key handling have been added:
privateKeysAtIndexPaths:fromSeed:derivationPath:serializedPrivateKeysAtIndexPaths:fromSeed:derivationPath:serializedExtendedPublicKey:- etc.
These methods would benefit from clear documentation explaining:
- Their purpose
- Parameter requirements
- Return value semantics
- Memory management expectations, especially for methods returning
DMaybeOpaqueKeys *
| + (NSString *)standaloneExtendedPublicKeyLocationStringForUniqueID:(NSString *)uniqueID; | ||
| + (NSString *)standaloneInfoDictionaryLocationStringForUniqueID:(NSString *)uniqueID; | ||
| + (NSString *)walletBasedExtendedPublicKeyLocationStringForUniqueID:(NSString *)uniqueID; | ||
| + (NSString *)walletBasedExtendedPrivateKeyLocationStringForUniqueID:(NSString *)uniqueID; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide details on uniqueID format and generation.
The new methods for generating location strings based on uniqueID:
+ (NSString *)standaloneExtendedPublicKeyLocationStringForUniqueID:(NSString *)uniqueID;
+ (NSString *)standaloneInfoDictionaryLocationStringForUniqueID:(NSString *)uniqueID;
+ (NSString *)walletBasedExtendedPublicKeyLocationStringForUniqueID:(NSString *)uniqueID;
+ (NSString *)walletBasedExtendedPrivateKeyLocationStringForUniqueID:(NSString *)uniqueID;Would benefit from documentation explaining:
- The expected format of uniqueID
- How uniqueIDs are generated
- The format and purpose of the resulting location strings
| - (void)unregisterWallet:(DSWallet *)wallet { | ||
| NSAssert(wallet.chain == self, @"the wallet you are trying to remove is not on this chain"); | ||
| [wallet wipeBlockchainInfoInContext:self.chainManagedObjectContext]; | ||
| [wallet wipeWalletInfo]; | ||
| [self.mWallets removeObject:wallet]; | ||
| NSError *error = nil; | ||
| NSMutableArray *keyChainArray = [getKeychainArray(self.chainWalletsKey, @[[NSString class]], &error) mutableCopy]; | ||
| if (!keyChainArray) keyChainArray = [NSMutableArray array]; | ||
| [keyChainArray removeObject:wallet.uniqueIDString]; | ||
| setKeychainArray(keyChainArray, self.chainWalletsKey, NO); | ||
| [self notify:DSChainWalletsDidChangeNotification userInfo:@{DSChainManagerNotificationChainKey: self}]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve keychain error handling when unregistering a wallet.
If getKeychainArray returns an error, it’s recovered by allocating a fresh array, which might mask the error. Consider at least logging or reporting the error to ensure users/devs are aware of potential keychain issues.
99 [keyChainArray removeObject:wallet.uniqueIDString];
100 setKeychainArray(keyChainArray, self.chainWalletsKey, NO);
+ else {
+ // Log the error or propagate it; avoid silently overriding the keychain array
+ DSLog(@"Keychain array retrieval error: %@", error);
+ }Committable suggestion skipped: line range outside the PR's diff.
|
We shouldn't merge develop now since it was updated with the new dash-shared-core integration. |
Issue being fixed or feature implemented
Now that CoinJoin is released, we should update the DashSharedCore version and merge it to
master.What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
I'll help you craft concise release notes based on the comprehensive summary. Given the extensive changes, I'll focus on the most significant user-facing updates.
Release Notes for DashSync v22
New Features
Improvements
Changes
Technical Updates
Breaking Changes
Compatibility