TransactionHelper deserialization #406

fboucquez · 2019-12-27T17:17:14Z

This PR is the SDK improvement from creating a catbuffer library via the generator. PR https://github.com/nemtech/catbuffer-generators/pull/67

If you are happy with the solution, we can release a proper catbuffer version (like 2.0.0) and update the current catbuffer lib version which is 0.0.4-SNAPSHOT

Removed catbuffer code
Added catbuffer dependency
Improved serialization to use TransactionHelper and builders when they are already parsed.
Added tslint task to npm (codestyle fixes still need fixing. Then we can enable Travis to do the checking)

Added catbuffer dependency Improved serialization to use TransactionHelper and builders when they are already parsed. Added tslint task to npm (issue needs to be fixed)

rg911

Similar to the PR review comments left on buffer generator. The NPM Package staffs are really nice. Great work. But the TransactionHelper will introduce breaking changes to the SDK. The InnerTransaction is already an exported class / type. We should evaluate the impact to the usage. Can the TransactionHelper be put into a separate PR so we can get this catbuffer package PR merged first? We should keep a PR to solve a single issue.

fboucquez · 2019-12-29T19:35:30Z

src/core/utils/TransactionMapping.ts

-    public static createFromPayload(payload: string,
-                                    isEmbedded = false): Transaction | InnerTransaction {
-        return CreateTransactionFromPayload(payload, isEmbedded);
+    public static createFromPayload(payload: string): Transaction {


Hi @rg911

I believe this is the only public API method that breaks. The removal of the optional isEmbedded. Alhotugh I don't think clients would be calling this method with isEmbedded = true, I can add the isEmbedded flag back and return InnerTransaction like the old signature. What do you think? Happy to split the PR if you still find it necessary.
This PR doesn't remove the InnerTransaction. That would be a different one once we get confirmation from the clients.

I tend to agree but am not 100% convinced using Transaction as EmbeddedTransaction. Code will work but the concept of EmbeddedTransaction becomes a confusing. @evias @dgarcia360

But simply from PR point of view, it should be split

Fair enough, I'll create a PR with just the catbuffer library and the current master code (I'll remove catbuffer code, include the package and just update the imports)

I'll keep this PR for the TransactonHelper usage. Should we remove the InnerTransaction here?

@evias @dgarcia360 , we are noticing that InnertTransaciton type doesn't add anything special to Transaction. The only added attribute signer is already in Transaction superclass. We want to remove it simplifying the models. It's a small breaking change. You would need to change any reference from InnerTransaction to Transaction.

not sure that many people would be using InnerTransaction much vs specialized TransferTransaction with call to toAggregate().

Thanks @evias ,

would you confirm I can remove InnerTransaction?

Is the simplier TransactionMapping method ok?

public static createFromPayload(payload: string): Transaction

I couldnt confirm the removal of InnerTransaction completely because this class may also be used when trying to read things from chain inside aggregates and I cant speak for all clients that this is required/not required.

Take following two payloads:

2.1: Simple transfer transaction (extends Transaction)

CD00000000000000A775C5ACE3D7167447629EB7BB535D87E6252D46B550E4E98EBF16B3815F60BC47080010FD3C3817620C49B2E50AB3562A88968A0244BF17DDDB268A2CFF220130664BF316E6CC84B668D283F15F783724640FD4CFD1229D7D9C7604EDF46805000000000198544140420F00000000004A941B050100000098DAD0DFE1E7049C9606A7EE037B528493075B1C7F540161E3011D000000000029CF5FD941AD25D501000000000000000053696D706C65207472616E73666572206F662031206E656D2E78656D

2.2: Aggregate transaction with one transfer transaction (extends Transaction)

1001000000000000D29C33F6983DA4DF650E3C5E638494C8620DF7A28C73E375917C92958B0A139FA4EE95F7175B7590811E0440CEF418D85D440C323D70571DC514DFC4A3CD260830664BF316E6CC84B668D283F15F783724640FD4CFD1229D7D9C7604EDF46805000000000198414140420F00000000005A831C05010000003E04C29404A49C65964BE1FFBB7E7EB7918D6211430EE8F8531BF5995175682F6800000000000000610000000000000030664BF316E6CC84B668D283F15F783724640FD4CFD1229D7D9C7604EDF46805000000000198544198DAD0DFE1E7049C9606A7EE037B528493075B1C7F540161E30101000000000029CF5FD941AD25D501000000000000000000000000000000

2.3: Embedded transaction content of one transfer transaction (extends InnerTransaction)

98DAD0DFE1E7049C9606A7EE037B528493075B1C7F540161E30101000000000029CF5FD941AD25D501000000000000000000000000000000

One thing I can think about is that removing the isEmbedded flag in the case of aggregate transactions, a client could share only the embedded payload (like in 2.3) so that other clients have correct content (but its not defined which key should sign). As a compromise, you could also provide with a createFromInnerPayload method.

I would remove it, it won't be a big change for the customers as it just changing the class name. Currently, the downcasting is pretty odd at least

I'll add the createFromInnerPayload in TransactionMapping

regarding all these changes mentioned, why can't it just keep as it is now? Doesn't seem to me a 'must' ?

I'm fine either way. 2 methods or 1 method with an if. I guess keeping the same signature won't break existing customers. (If the code would have been new, I would prefer 2 methods though)

Removed InnerTransaction. Code is simpler.
Added isEmbedded back to the createFromPayload
Added unit test

fboucquez · 2019-12-29T20:42:35Z

Created #408 that only includes the catbuffer library.

This PR will just include the TransactionHelper improvement and probably the removal of InnerTransaction once users are happy.

evias

would recommend a minor version upgrade for these changes. anyways great work 👍

One thing I notice where I will review tomorrow again is the breaking change with embedded transactions 👍

evias · 2019-12-30T19:21:27Z

src/core/utils/TransactionMapping.ts

-    public static createFromPayload(payload: string,
-                                    isEmbedded = false): Transaction | InnerTransaction {
-        return CreateTransactionFromPayload(payload, isEmbedded);
+    public static createFromPayload(payload: string): Transaction {


not sure that many people would be using InnerTransaction much vs specialized TransferTransaction with call to toAggregate().

# Conflicts: # package-lock.json # package.json # src/infrastructure/transaction/CreateTransactionFromPayload.ts # src/model/transaction/AggregateTransaction.ts # src/model/transaction/TransferTransaction.ts

Added isEmbedded argument back to TransactionMapping Fixed catbuffer lib version using the one that includes the src and dist folders for easy debugging.

# Conflicts: # package.json # src/model/transaction/TransactionStatus.ts

# Conflicts: # src/infrastructure/transaction/CreateTransactionFromPayload.ts # src/model/transaction/AccountAddressRestrictionTransaction.ts # src/model/transaction/AccountMosaicRestrictionTransaction.ts # src/model/transaction/AccountOperationRestrictionTransaction.ts # src/service/AggregateTransactionService.ts

…pescript-javascript into catbuffer-library

…ion handling and easier to understand) Improved how catapult-service-bootstrap folder is found

Removed catbuffer code

3772c4a

Added catbuffer dependency Improved serialization to use TransactionHelper and builders when they are already parsed. Added tslint task to npm (issue needs to be fixed)

fboucquez requested a review from rg911 December 27, 2019 17:17

Upgraded catbuffer library. Improved imports

fea5ac1

rg911 suggested changes Dec 29, 2019

View reviewed changes

fboucquez commented Dec 29, 2019

View reviewed changes

fboucquez changed the title ~~Catbuffer library~~ TransactionHelper deserialization Dec 29, 2019

fboucquez mentioned this pull request Dec 29, 2019

Catbuffer library #408

Merged

evias reviewed Dec 30, 2019

View reviewed changes

fboucquez and others added 3 commits December 30, 2019 18:31

Merge remote-tracking branch 'origin/master' into catbuffer-library

aa66743

# Conflicts: # package-lock.json # package.json # src/infrastructure/transaction/CreateTransactionFromPayload.ts # src/model/transaction/AggregateTransaction.ts # src/model/transaction/TransferTransaction.ts

Merge branch 'master' into catbuffer-library

5587cb3

Removed InnerTransaction

127d9da

Added isEmbedded argument back to TransactionMapping Fixed catbuffer lib version using the one that includes the src and dist folders for easy debugging.

fboucquez requested review from evias and rg911 December 31, 2019 17:46

fboucquez added 5 commits January 29, 2020 13:15

Merge remote-tracking branch 'origin/master' into catbuffer-library

2f7f516

# Conflicts: # package.json # src/model/transaction/TransactionStatus.ts

Merged master

173b6cd

Merge branch 'catbuffer-library' of github.com:NEMStudios/nem2-sdk-ty…

1de9bf4

…pescript-javascript into catbuffer-library

Migreated done callbacks to async await in e2e testing (better except…

39bbb21

…ion handling and easier to understand) Improved how catapult-service-bootstrap folder is found

rg911 approved these changes Jan 30, 2020

View reviewed changes

rg911 merged commit c92f39d into symbol:master Jan 30, 2020

fboucquez deleted the catbuffer-library branch April 13, 2020 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TransactionHelper deserialization #406

TransactionHelper deserialization #406

Uh oh!

fboucquez commented Dec 27, 2019 •

edited

Loading

Uh oh!

rg911 left a comment

Uh oh!

fboucquez Dec 29, 2019 •

edited

Loading

Uh oh!

rg911 Dec 29, 2019

Uh oh!

fboucquez Dec 29, 2019 •

edited

Loading

Uh oh!

evias Dec 30, 2019

Uh oh!

fboucquez Dec 30, 2019

Uh oh!

evias Dec 31, 2019

Uh oh!

fboucquez Dec 31, 2019 •

edited

Loading

Uh oh!

rg911 Dec 31, 2019 •

edited

Loading

Uh oh!

fboucquez Dec 31, 2019 •

edited

Loading

Uh oh!

fboucquez Dec 31, 2019

Uh oh!

fboucquez commented Dec 29, 2019

Uh oh!

evias left a comment

Uh oh!

evias Dec 30, 2019

Uh oh!

Uh oh!

TransactionHelper deserialization #406

TransactionHelper deserialization #406

Uh oh!

Conversation

fboucquez commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rg911 left a comment

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rg911 Dec 29, 2019

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evias Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

evias Dec 31, 2019

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rg911 Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fboucquez Dec 31, 2019

Choose a reason for hiding this comment

Uh oh!

fboucquez commented Dec 29, 2019

Uh oh!

evias left a comment

Choose a reason for hiding this comment

Uh oh!

evias Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fboucquez commented Dec 27, 2019 •

edited

Loading

fboucquez Dec 29, 2019 •

edited

Loading

fboucquez Dec 29, 2019 •

edited

Loading

fboucquez Dec 31, 2019 •

edited

Loading

rg911 Dec 31, 2019 •

edited

Loading

fboucquez Dec 31, 2019 •

edited

Loading