Task/g357 standardize transaction service #363
Conversation
- Updated aggregate transaction handler with secondary source id
Task/g361 network in http
- Updated aggregate transaction handler with secondary source id
…m:NEMStudios/nem2-sdk-typescript-javascript into task/g357_standardize_transaction_service
| * @param aggregateTransactionIndex Transaction index for aggregated transaction | ||
| * @returns {Observable<AccountAddressRestrictionTransaction>} | ||
| */ | ||
| resolveAliases(receiptHttp: ReceiptHttp, aggregateTransactionIndex?: number): Observable<AccountAddressRestrictionTransaction> { |
There was a problem hiding this comment.
I am not fan of the double dispatch. It gets the Transaction model too complicated with internal methods the user doesn't really care. I would create a switch/handers statement in the TransactionService (especially when there is a static call TransactionService.getResolvedFromReceipt) and remove resolveAliases from the transaction hierarchy.
I would stop using the Http implementations and start using the Repository Interface, in this case, ReceiptRepository
There was a problem hiding this comment.
Do you mean to put all codes in to one class here? It's another option agree but the single class can be complicated and main thing here is you will lose the single responsibility for Transaction to resolve alias.
Regrading the interface, ts doesn't work like JAVA or C# which you can use unity or spring, There's no DI in TS SDK, so resolving the interface is not easy
There was a problem hiding this comment.
Not about DI, it's just about using interfaces, you can do
resolveAliases(recipientRepository: RecipientRepository, aggregateTransactionIndex?: number)
There was a problem hiding this comment.
How do you find the correct implementation of the injected interface?
There was a problem hiding this comment.
Well done @rg11, now it looks complete 👍 I can't think of any other edge case, it would be great to have a second review before merging the PR.
5 participants
ResolutionStatementPlease note that there's discussion on NEM2 slack channel regarding the
PrimaryIdvalue inResolutionStatementas ifHarvest_Feereceipt exists on the block, the transaction index is less1against theResolution entry primaryId. Solution here may change upon investigation results on catapult-serverIssue: #357