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

Implement create and sign any transaction functionality in Lisk Transactions - Closes #3739 #3834

Closed
wants to merge 2 commits into from

Conversation

MaciejBaj
Copy link
Contributor

What was the problem?

There wasn't a functionality allowing to create and sign any transaction extending BaseTransaction in an easy way.

Review checklist

elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/src/create_sendable.ts Outdated Show resolved Hide resolved
elements/lisk-transactions/package-lock.json Outdated Show resolved Hide resolved
Copy link
Contributor

@lsilvs lsilvs left a comment

Choose a reason for hiding this comment

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

What is the reason to have this createSendable function?
All it does is validate, instantiate and sign transactions. The validation should be during instantiation imo which would make this function to only instantiate and sign the transactions.

recipientId:
recipientId || getAddressFromPublicKey(recipientPublicKey)
senderPublicKey:
senderPublicKey || getPrivateAndPublicKeyFromPassphrase(passphrase),
Copy link
Contributor

Choose a reason for hiding this comment

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

senderPublicKey will be an object with public and private key if senderPublicKey === null


transaction.sign(passphrase, secondPasshprase);

return JSON.stringify(skipUndefined(transaction.toJSON()));
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON.stringify removes keys with undefined values.
No need to use skipUndefined here.

@MaciejBaj MaciejBaj changed the title Implement create and sign any transaction functionality in Lisk Transactions Implement create and sign any transaction functionality in Lisk Transactions - Closes #3739 Jun 26, 2019
Signed-off-by: Maciej Baj <macie.baj@gmail.com>
Signed-off-by: Maciej Baj <macie.baj@gmail.com>
Copy link
Member

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

What is the purpose of this PR?
It seems more like a refactoring, and it doesn't reflect the title of the PR

@@ -0,0 +1,52 @@
import { getAddressFromPublicKey } from '@liskhq/lisk-cryptography';
Copy link
Member

Choose a reason for hiding this comment

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

import should come after license

export const decideOnRecipientId = (
recipientId?: string | null,
recipientPublicKey?: string,
noValue = '',
Copy link
Member

Choose a reason for hiding this comment

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

it seems doesn't need this input.
I think we can just return empty string in the last line.
same for below

@@ -221,6 +219,10 @@ export abstract class BaseTransaction {
return transaction;
}

public stringify(): string {
Copy link
Member

Choose a reason for hiding this comment

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

what's the reasoning to add this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

console.log(new MyTransaction(...).sign('my passphrase').sendable())

Copy link
Member

Choose a reason for hiding this comment

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

you mean console.log(new MyTransaction(...).sign('my passphrase').stringify())?
when should it be used like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I meant this, before sending :)

*
*/

export const decideOnRecipientId = (
Copy link
Member

Choose a reason for hiding this comment

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

decideOnRecipientId doesn't describe what it's doing.
Maybe safeGetRecipientId? (which doesn't sounds good too)

@@ -13,6 +13,7 @@
*
*/
export * from './address';
export * from './constructor';
Copy link
Member

Choose a reason for hiding this comment

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

constructor is reserved word, it should use other name.
safe_get_ids might be better? depends on the function names

}
// Obtain senderId out of senderPublicKey if no recipientId
if (senderPublicKey) {
return getAddressFromPublicKey(senderPublicKey);
Copy link
Member

Choose a reason for hiding this comment

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

this could fail and throw an error, same as above

@MaciejBaj
Copy link
Contributor Author

Proceeded by #3890.

@MaciejBaj MaciejBaj closed this Jul 2, 2019
@shuse2 shuse2 deleted the 3739-create_sendable branch July 29, 2019 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants