refactor(ckb): encapsulate transaction fee supplementation methods #302
Conversation
duanyytop
requested review from
duanyytop,
ShookLyngs and
Flouse
and removed request for
duanyytop
packages/ckb/src/utils/ckb-tx.ts
Outdated
| skipMissingKeys?: boolean, | ||
| ): CKBComponents.RawTransaction; | ||
|
|
||
| export function signCkbTransaction( |
There was a problem hiding this comment.
Why are there three functions with the same name signCkbTransaction?
There was a problem hiding this comment.
The first overload handles the case where the key is a single string and inputCells is optional. The second overload is for when key is a Map<string, string>, in which case inputCells is required. The third definition is the actual implementation. I believe these overloads improve type checking and IDE support by explicitly defining valid parameter combinations, even though they do add some complexity.
There was a problem hiding this comment.
Can you verify if it works if there is only one function signature?
There was a problem hiding this comment.
Yes, the third function, which is the actual implementation, is sufficient on its own. The following are the related transactions I just sent for xUDT testing on Testnet3 in offline mode:
- Issuance preparation
- Distribution
There was a problem hiding this comment.
So can we remove the above two functions?
There was a problem hiding this comment.
Sure, I'll make the change.
There was a problem hiding this comment.
Addressed in 740612b, along with this suggested change #302 (comment).
| */ | ||
| export const appendIssuerCellToBtcBatchTransfer = async ({ | ||
| secp256k1PrivateKey, | ||
| export const appendOwnerCellToRgbppTx = async ({ |
There was a problem hiding this comment.
Suggestion: appendOwnerCellToRgbppTx -> appendIssuerCellToBtcBatchTransferToSign
packages/ckb/src/rgbpp/btc-time.ts
Outdated
| */ | ||
| export const signBtcTimeCellSpentTx = async ({ | ||
| secp256k1PrivateKey, | ||
| export const completeBtcTimeCellSpentTx = async ({ |
There was a problem hiding this comment.
Suggestion: completeBtcTimeCellSpentTx -> buildBtcTimeCellSpentTx
There was a problem hiding this comment.
I'm not sure if this change is a good idea since the suggested function name is very similar to buildBtcTimeCellsSpentTx, which might cause confusion.
There was a problem hiding this comment.
Yes, you are right. Maybe prepareBtcTimeCellSpentUnsignedTx is better?
packages/ckb/src/spore/spore.ts
Outdated
| */ | ||
| export const appendIssuerCellToSporesCreate = async ({ | ||
| secp256k1PrivateKey, | ||
| export const completeAppendingIssuerCellToSporesCreateTx = async ({ |
There was a problem hiding this comment.
Suggestion: completeAppendingIssuerCellToSporesCreateTx -> appendIssuerCellToSporesCreateUnsignTx
There was a problem hiding this comment.
Suggestion: appendIssuerCellToSporesCreateUnsignTx -> appendIssuerCellToSporesCreateUnsignedTx
packages/ckb/src/utils/ckb-tx.ts
Outdated
| }; | ||
|
|
||
| // Normalizes a CKB transaction to be signed by ensuring all witnesses are in serialized string format. | ||
| export const normalizeCkbTxToSign = (ckbTx: CKBComponents.RawTransactionToSign): CKBComponents.RawTransactionToSign => { |
There was a problem hiding this comment.
Is there any function calling function normalizeCkbTxToSign?
There was a problem hiding this comment.
Not in the current codebase. This function was implemented to ensure that RawTransactionToSign.witness is consistently represented as a string, simplifying deserialization in other strongly typed languages.
There was a problem hiding this comment.
Sorry, I don't understand.
Why do we keep this function if no other functions call the normalizeCkbTxToSign? And the signCkbTransaction already has the same code.
If you want a reference for other programming languages on how to do it, adding more comments in the signCkbTransaction function can work too.
There was a problem hiding this comment.
Agreed! Adding comments in signCkbTransaction would be a better approach.
Extract the logic for supplementing transaction fees into separate methods to facilitate users handling the signing logic for the final CKB transaction on their own.
xUDT testing transactions on Testnet3 in offline mode:
xUDT testing transactions on Testnet3 in default mode:
Spore Creation: