Skip to content

Conversation

@rg911
Copy link
Contributor

@rg911 rg911 commented Sep 11, 2019

  • Latest E3 schema changes applied
  • Catbuffer builder E3 compatible
  • Http client generated code (OpenApi3) E3 compatible
  • Mosaic property refactored (Removed properties array, make duration mandatory)
  • Changed Http Repository to use Address for all accountIds (Address | PubKey) for rest endpoint querying. E.g. accountHttp.getTransactions(). Didn't make the parameter optional, as Address class has the factory methods to build from address or publicKeys (Address.createFromRawAddress, Address.createFromPublicKey).
  • Removed 'IsAnnouncedcheck inCosignTransactions` to allow offline aggregateTransaction cosigning.

Issue resolved:
#240 #223 #228 #213

@rg911 rg911 requested review from dgarcia360 and evias September 11, 2019 20:55
@coveralls
Copy link

coveralls commented Sep 11, 2019

Pull Request Test Coverage Report for Build 650

  • 793 of 1550 (51.16%) changed or added relevant lines in 211 files are covered.
  • 13 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.8%) to 68.933%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/infrastructure/catbuffer/EmbeddedMosaicDefinitionTransactionBuilder.ts 1 2 50.0%
src/infrastructure/catbuffer/EmbeddedMosaicSupplyChangeTransactionBuilder.ts 1 2 50.0%
src/infrastructure/catbuffer/EmbeddedNamespaceRegistrationTransactionBuilder.ts 1 2 50.0%
src/infrastructure/model/accountAddressRestrictionModificationDTO.ts 3 4 75.0%
src/infrastructure/model/accountAddressRestrictionTransactionBodyDTO.ts 3 4 75.0%
src/infrastructure/model/accountAddressRestrictionTransactionDTO.ts 3 4 75.0%
src/infrastructure/model/accountLinkTransactionBodyDTO.ts 3 4 75.0%
src/infrastructure/model/accountLinkTransactionDTO.ts 3 4 75.0%
src/infrastructure/model/accountMosaicRestrictionModificationDTO.ts 3 4 75.0%
src/infrastructure/model/accountMosaicRestrictionTransactionBodyDTO.ts 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
src/infrastructure/AccountHttp.ts 1 25.49%
src/infrastructure/api/blockRoutesApi.ts 1 5.72%
src/infrastructure/api/namespaceRoutesApi.ts 1 3.7%
src/infrastructure/catbuffer/CosignatureBuilder.ts 1 16.67%
src/infrastructure/catbuffer/EmbeddedAddressAliasTransactionBuilder.ts 1 16.67%
src/infrastructure/catbuffer/EmbeddedHashLockTransactionBuilder.ts 1 16.67%
src/infrastructure/catbuffer/EmbeddedMosaicAliasTransactionBuilder.ts 1 16.67%
src/model/mosaic/MosaicId.ts 1 80.0%
src/infrastructure/TransactionHttp.ts 2 23.86%
src/infrastructure/api/accountRoutesApi.ts 3 7.29%
Totals Coverage Status
Change from base Build 641: 0.8%
Covered Lines: 6289
Relevant Lines: 8400

💛 - Coveralls

@fboucquez fboucquez self-requested a review September 13, 2019 13:39
Copy link
Contributor

@fboucquez fboucquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Steve, I have added several comments. Some are improvements for a future task. Some you may be able to fix in this PR (for example adding a method to centralize how NamespaceId are mapped form a Json field).

* @returns Observable<Transaction[]>
*/
unconfirmedTransactions(publicAccount: PublicAccount,
unconfirmedTransactions(address: Address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, an Address is simpler to create than a PublicAccount. This simplifies the api for the sdk client.

@fboucquez fboucquez self-requested a review September 16, 2019 09:23
Copy link
Contributor

@fboucquez fboucquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the latest changes Steve

@rg911 rg911 merged commit 2601e19 into symbol:master Sep 16, 2019
@rg911 rg911 deleted the task/g240_elephant3_schema_changes branch October 8, 2019 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants