Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing classes to typings #30

Open
wants to merge 4 commits into
base: development
from

Conversation

@The-Cat-In-The-Hat
Copy link

The-Cat-In-The-Hat commented Dec 7, 2019

No description provided.

Copy link
Collaborator

brandonlehmann left a comment

I see you changed a few export names in the CryptoNote class. I’m not sure if this will be a breaking change for downstream packages. If it is, the current export names must remain.

@The-Cat-In-The-Hat

This comment has been minimized.

Copy link
Author

The-Cat-In-The-Hat commented Dec 7, 2019

I added some type aliases. Is it ok now?

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Dec 10, 2019

I added some type aliases. Is it ok now?

Let me, um, phone a friend since typescript isn't my thing.

@zpalmtree do you mind making sure this doesn't break anything up your way?

@zpalmtree

This comment has been minimized.

Copy link
Contributor

zpalmtree commented Dec 10, 2019

I added some type aliases. Is it ok now?

Let me, um, phone a friend since typescript isn't my thing.

@zpalmtree do you mind making sure this doesn't break anything up your way?

I get a couple of type errors.

  1. Output expects a transactionKey and publicEphemeral property, however those properties are not used in createTransaction, so I'm getting a type error from them being missing. Perhaps they should be updated in both docs to be an optional property?

  2. transactionKeys is present on the transaction object returned from createTransaction, but this is not present in the jsdoc nor typescript

  3. amount is now potentially undefined, which is causing an error, but this is a failure of the previous documentation imo, so doesn't need rectifying.

  4. createTransaction is documented in the jsdoc and typescript as returning boolean | CreatedTransaction, but I'm not seeing a codepath which returns a boolean.

I see @The-Cat-In-The-Hat has fixed these type errors on the pull request he made to the backend, but obviously there may be other typescript users out there.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Dec 10, 2019

I added some type aliases. Is it ok now?

Let me, um, phone a friend since typescript isn't my thing.
@zpalmtree do you mind making sure this doesn't break anything up your way?

I get a couple of type errors.

  1. Output expects a transactionKey and publicEphemeral property, however those properties are not used in createTransaction, so I'm getting a type error from them being missing. Perhaps they should be updated in both docs to be an optional property?

Which Output?

  1. transactionKeys is present on the transaction object returned from createTransaction, but this is not present in the jsdoc nor typescript

Added to jsdoc

  1. amount is now potentially undefined, which is causing an error, but this is a failure of the previous documentation imo, so doesn't need rectifying.

Please link?

  1. createTransaction is documented in the jsdoc and typescript as returning boolean | CreatedTransaction, but I'm not seeing a codepath which returns a boolean.

Corrected in jsdoc

I see @The-Cat-In-The-Hat has fixed these type errors on the pull request he made to the backend, but obviously there may be other typescript users out there.

@zpalmtree

This comment has been minimized.

Copy link
Contributor

zpalmtree commented Dec 10, 2019

Which Output?

Apologies, I see now it's GeneratedInput on the input of Output.

Please link?

I use the amount returned by the GeneratedTransaction , then the Transaction object on that, then the inputs array on that, which has an optional amount property since it's only present on 02 inputs.

@The-Cat-In-The-Hat The-Cat-In-The-Hat force-pushed the The-Cat-In-The-Hat:development branch from 8f72488 to e79470c Jan 3, 2020
@The-Cat-In-The-Hat The-Cat-In-The-Hat force-pushed the The-Cat-In-The-Hat:development branch from e79470c to f808c93 Jan 3, 2020
@The-Cat-In-The-Hat

This comment has been minimized.

Copy link
Author

The-Cat-In-The-Hat commented Jan 3, 2020

Sorry for delayed response. I see the transactionKeys property is now present on the Transaction class, but not on the GeneratedTransaction which is what createTransaction returns. I believe I have updated all the issues flagged up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.