Skip to content

Conversation

decentraliser
Copy link
Contributor

sorry about this one...
sorted the exports along the way

@evias
Copy link
Contributor

evias commented Dec 27, 2019

is there no other DTOs that need to be exported ?

Kind of a bad thing to do to expose one specific class in there, I would prefer it to expose a DTO submodule (with any required DTOs)

@decentraliser
Copy link
Contributor Author

The other DTOs are DTO that are used for typing REST API results, I separated this one from these upon demand of @rg911 for not impacting OpenAPI documentation automatic generation

Copy link
Contributor

@rg911 rg911 left a comment

Choose a reason for hiding this comment

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

Still think the simpleWalletDTO is client specific model. Question is should we need to add it to the SDK or keep it @internal or move it into the wallet project completely?

@fboucquez
Copy link
Contributor

I agree with Steve. I think this PR belongs to the wallet project. I don't think other apps may care about it.

@decentraliser
Copy link
Contributor Author

decentraliser commented Dec 27, 2019

@fboucquez
Copy link
Contributor

Should the wallet be part of the NEM cli?

@decentraliser
Copy link
Contributor Author

In the end, it is just a small detail.

Maybe I don't know what the "wallet project" is?

In my opinion, this should be provided by the SDK, here is my reasoning:

  • The SDK provides the SimpleWallet class, with handy methods to encrypt and decrypt a private key
  • the only use case for a simple wallet is to be stored and retrieved
  • if the SDK provides a SimpleWallet method, it should cover its full lifecycle

it's not only about the CLI, but this is also used in the desktop wallet, the browser extension wallet, and I guess in the mobile wallet as well

@rg911
Copy link
Contributor

rg911 commented Dec 29, 2019

In the end, it is just a small detail.

Maybe I don't know what the "wallet project" is?

In my opinion, this should be provided by the SDK, here is my reasoning:

  • The SDK provides the SimpleWallet class, with handy methods to encrypt and decrypt a private key
  • the only use case for a simple wallet is to be stored and retrieved
  • if the SDK provides a SimpleWallet method, it should cover its full lifecycle

it's not only about the CLI, but this is also used in the desktop wallet, the browser extension wallet, and I guess in the mobile wallet as well

Okay, in that case, like @evias suggested, could a submodule be created? and we already have a wallet folder in model, can it be put with other wallet classes together?

@decentraliser decentraliser requested a review from rg911 December 30, 2019 07:55
@rg911 rg911 merged commit b5ce1df into symbol:master Dec 31, 2019
@decentraliser decentraliser deleted the simple-wallet branch February 20, 2020 15:32
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