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

feat(core): add transaction builder for signing transactions with mul… #362

Merged
merged 1 commit into from
Oct 12, 2019

Conversation

Keith-CY
Copy link
Member

@Keith-CY Keith-CY commented Oct 8, 2019

…tiple private keys

@Keith-CY Keith-CY marked this pull request as ready for review October 9, 2019 06:25
import { blake2b, hexToBytes, bytesToHex, PERSONAL } from '@nervosnetwork/ckb-sdk-utils'
import Address from './address'

export default (addrObj: Address, transactionHash: string, witness: string = '') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

signWitness is a action, so maybe it's not appropriate for file name.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's better to keep the name the same as the function exported by default in js.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

s.update(hexToBytes(transactionHash))
s.update(witnessBytes)
const message = `0x${s.digest('hex')}`
const data = [...hexToBytes(addrObj.signRecoverable(message)), ...witnessBytes]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why signRecoverable function is in Address module ?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, the Address class derives from the ECPairs class, which is used to generate key pairs, thus the address object is able to sign messages.

Copy link
Contributor

@duanyytop duanyytop Oct 9, 2019

Choose a reason for hiding this comment

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

OK. I just think Address and ECPairs are different modules, and signRecoverable can be in ECPairs but not Address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. I have been wanting to propose changing Address's base type for long time. It's not great to let developer know that address is an ECPairs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please plan this in another feature branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha

}
}

public updateTransactionHash = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

When and where updateTransactionHash function will be called? I don't catch it and I think it could be private.

Copy link
Member Author

@Keith-CY Keith-CY Oct 9, 2019

Choose a reason for hiding this comment

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

It will be called automatically on calling signInput if the builder.hash is empty. But it should be able to update the hash manually when the raw transaction is updated and the hash is calculated already.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think sometimes we support more interfaces maybe not a good thing.

@ashchan ashchan merged commit 41048a3 into develop Oct 12, 2019
@ashchan ashchan deleted the support-multi-private-keys-in-tx branch October 12, 2019 06:45
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.

None yet

3 participants