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

Refactor: remove ckb-sdk-utils and repalce with lumos #2734

Closed
wants to merge 6 commits into from

Conversation

zhangyouxin
Copy link
Contributor

@zhangyouxin zhangyouxin commented Jun 20, 2023

Description

Due to merge of ckb-sdk-x to lumos, we need to gradually deprecate ckb-sdk-x, this PR removes ckb-sdk-utils in packages/neuron-wallet, It's not a small PR and may be not that easy to review, so I plan to seperate this task to two PRs, will create another PR to remove it in packages/neuron-ui

This PR is not not compilable, because I have updated lumos to 0.20, some snake_case fields has changed to camelCase, the task is in another PR: #2683, only when after that PR is merged, this PR can be ready for review.

Resolve Magickbase/neuron-public-issues#201

changed

  • scriptToHash(script) should be replaced with utils.computeScriptHash(script).
  • scriptToAddress(script, isMainet) should be replaced with helpers.encodeToAddress(script, lumosOptions). We should translate isMainet to lumosOptions here.
  • addressToScript(address) should be replaced with helpers.addressToScript(address, lumosOptions). We should detect if isMainet by checking if the address starts with 'ckb' or not. Alternatively, we could use NetworksService.getInstance().isMainnet().
  • src/models/blake2b.ts has been deleted and replaced with @ckb-lumos/hd and lumos helpers.
  • null values are not compatible with @ckb-lumos/base types.
  • Some test cases have been changed:
    • args should be a valid hex string. Values like "0xuuid" have been deleted.
    • Optional hex values should be 0x or undefined. Empty strings have been deleted.
    • Hex strings are not hex numbers. Values like 0x1 have been changed to 0x01.

@homura
Copy link
Collaborator

homura commented Jun 27, 2023

@zhangyouxin the conflicts is needed to resolve

@github-actions
Copy link
Contributor

Packaging for test is done in 5390477118 for commit 8e89627 .

@github-actions
Copy link
Contributor

Packaging for test is done in 5396537909 for commit 7ffd05c .

@zhangyouxin zhangyouxin marked this pull request as ready for review June 28, 2023 03:16
@github-actions
Copy link
Contributor

Packaging for test is done in 5396866191 for commit dd6a4de .

@@ -33,7 +33,8 @@ export default class TxAddressFinder {

const outputsResult: [boolean, Output[], AnyoneCanPayInfo[]] = this.selectOutputs()
const isMainnet = NetworksService.getInstance().isMainnet()
const outputAddresses: string[] = outputsResult[1].map(output => scriptToAddress(output.lock, isMainnet))
const lumosOptions = isMainnet ? { config: config.predefined.LINA } : { config: config.predefined.AGGRON4 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are so many isMainnet ? LINA : AGGRON4 in the PR, such a refactoring would only increase the complexity, maybe it is better to extract a helper to cover the case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, we can init the config after ckb node starts and change the config when the network changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

a migration that has already been released should not be refactored, which may pose some risks

Comment on lines +272 to +289
const rawTransaction = this.toSDKRawTransaction()
return utils.ckbHash(
blockchain.RawTransaction.pack({
...rawTransaction,
outputs: rawTransaction.outputs.map(o => ({
...o,
type: o.type || undefined,
})),
cellDeps: rawTransaction.cellDeps.map(cd => ({
...cd,
outPoint: cd.outPoint!,
})),
inputs: rawTransaction.inputs.map(i => ({
...i,
previousOutput: i.previousOutput!,
})),
})
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is found that refactoring increases complexity, this refactoring should not be done, and if there is no export method in lumos to calculate transaction hash, it should be marked as TODO instead of refactoring as such

Comment on lines +25 to +26
codeHash: config.predefined.LINA.SCRIPTS.DAO.CODE_HASH,
hashType: config.predefined.LINA.SCRIPTS.DAO.HASH_TYPE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

SystemScriptInfo may not same with the LINA's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The codeHashes of systemScripts are the same in mainnet/testnet/devnet

Comment on lines +33 to +40
const cellOutput = output.toSDK()
const bytes = bytesUtils.hexify(
blockchain.CellOutput.pack({
capacity: cellOutput.capacity,
lock: cellOutput.lock,
type: cellOutput.type || undefined, // TODO: null is not a packable type
})
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the TODO means here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cellOutput.type could be null, so maybe it's better to provide a translator to convert CKBCompont types with nulls to lumos compatible types with undefineds so that the code may look nicer.

const bytes = serializeFixVec(wit)
return HexUtils.byteLength(bytes) + TransactionSize.SERIALIZED_OFFSET_BYTESIZE
if (typeof witness === 'string') {
const fixvecCodec = fixvec(number.Uint8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

witness is not uint8, it is bytes, and I think the refactor just makes the code hard to understand

the blockchain.mol describes the Transaction as

table Transaction {
    raw:            RawTransaction,
    witnesses:      BytesVec,
}

vector Bytes <byte>;
vector BytesVec <Bytes>;

So it should be BytesVec.pack(Bytes.pack(WitnessArgs.pack({lock:'...'}))) here, but it is not. I think this method only calculates a part of the size, now this change will only make this method more difficult to understand, I suggest not to change this method first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this part of code is hard to understand. So I just try to keep the logic as it is.

The original code just wraps witness in Uint8Array by using serializeFixVec.

The idea you provided here is changing original logic, which may cause unintended bugs, please refer to :

public static witness(witness: WitnessArgs | string): number {
const wit: string = typeof witness === 'string' ? witness : serializeWitnessArgs(witness.toSDK())
const bytes = serializeFixVec(wit)
return HexUtils.byteLength(bytes) + TransactionSize.SERIALIZED_OFFSET_BYTESIZE
}

{
args,
codeHash: systemScripts.SECP256K1_BLAKE160.codeHash,
hashType: systemScripts.SECP256K1_BLAKE160.hashType,
codeHash: config.predefined.AGGRON4.SCRIPTS.SECP256K1_BLAKE160.CODE_HASH,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found there are so many AGGRON4 in the refactor, it is better to extract them to an independent module to manage the config, that would make the config easy to maintain when the config has changed or the code hash is not the same with the predefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this part, we are just using SECP256K1_BLAKE160 code hash, which is the same in mainnet/testnet/devnet.

Because lumos utils like helpers.encodeToAddress, parseAddress needs to receive a config, so I plan to create a static method to provide config like NeuronInstance.getLumosConfig()

@@ -63,8 +63,8 @@ describe('SUDTController', () => {
'txHash',
'0',
'10000',
Script.fromObject({ codeHash: '', args: '', hashType: ScriptHashType.Type }),
Script.fromObject({ codeHash: '', args: '', hashType: ScriptHashType.Type }),
Script.fromObject({ codeHash: `0x${'00'.repeat(32)}`, args: '0x', hashType: ScriptHashType.Type }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better to add some comment to describe why the empty codeHash is required here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have described the reason in the PR comment, please refer to:#2734 (comment)

@@ -310,7 +310,8 @@ describe('integration tests for AddressService', () => {

describe('#generateAndSaveForPublicKey', () => {
describe('with public key info exist for the public key', () => {
const publicKey = 'public key'
// const publicKey = 'public key' // not valid public key
const publicKey = '0x' + '00'.repeat(65) // 65 bytes pub key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just keep it as it is, unit tests can't always construct complete test data, as compared to semanticization, which makes unit tests easier to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't pass test after refactor, because publicKey is used to generate secp256k1 args, in lumos we can call utils.ckbHash(pubkey).slice(0, 42), which requires publicKey to be a valid BytesLike value.

@@ -12,6 +11,7 @@ import OfflineSignService from '../services/offline-sign'
import Multisig from '../models/multisig'
import SystemScriptInfo from '../models/system-script-info'
import NetworksService from '../services/networks'
import { config as lumosConig, helpers, utils, config } from '@ckb-lumos/lumos'
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference between config as lumosConfig and config

Copy link
Contributor Author

@zhangyouxin zhangyouxin Jun 29, 2023

Choose a reason for hiding this comment

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

setting lumosConfig as alias for config , because there is another viarable named config in the scope

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't get it, lumosConfig and config are both from @ckb-lumos/lumos.config, pointing to the same one.

BTW, lumosConig is a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const saveConfigs = Object.values(configOutput.multisig_configs).map(config => ({
r: +config.require_first_n,
m: +config.threshold,
n: config.sighash_addresses.length,
blake160s: config.sighash_addresses.map(v => addressToScript(v).args),
walletId,
alias: config.alias,
}))

config is delared here in line 110, if import { config } from '@ckb-lumos/lumos', then there are two configs in the mapFunction, I'll fix the typo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { config as lumosConig, helpers, utils, config } from '@ckb-lumos/lumos'
import { config as lumosConig, helpers, utils } from '@ckb-lumos/lumos'

@zhangyouxin
Copy link
Contributor Author

I find that totally removal of ckb-sdk-utils may be not as easy as I thougt of, and some changes needs to be discussed, so is it a better idea to seperate the task to more small tasks? @homura

After creating this PR, I can see some sub-tasks.

So I would like to close this PR and create each PR for:

  • config management
    • (discussion) how to manage a lumos config, or we don't need a full lumos config, just part of it
    • convert Script to Address
    • convert Address to Script
  • refactor Script class: packages/neuron-wallet/src/models/chain/script.ts
    • plan to remove this class, the methods of this class can be non-state, so it can be replaced by util functions
    • computeHash -> utils.computeScriptHash()
    • calculateBytesize -> blockchain.Script.pack(script).bytelength
  • Serialization/Deserialization for Script, RawTransaction, WitnessArgs
  • refactor: src/models/blake2b.ts
    • replace with CKBHasher in @ckb-lumos/base
  • Test cases
    • (discussion) some old conventions are not sutable for lumos style , such as script with args: "0xnot-hex" is not allowed when we use blockChain.Script.pack(script)

With the tasks seperated we can push the diliveration of this PR more smoothly

@@ -33,7 +33,8 @@ export default class TxAddressFinder {

const outputsResult: [boolean, Output[], AnyoneCanPayInfo[]] = this.selectOutputs()
const isMainnet = NetworksService.getInstance().isMainnet()
const outputAddresses: string[] = outputsResult[1].map(output => scriptToAddress(output.lock, isMainnet))
const lumosOptions = isMainnet ? { config: config.predefined.LINA } : { config: config.predefined.AGGRON4 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, we can init the config after ckb node starts and change the config when the network changes.

const script = addressToScript(address)
const isMainnet = address.startsWith('ckb')
const script = isMainnet
? helpers.addressToScript(address, { config: config.predefined.LINA })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the address.startsWith('ckb') logic to helpers.addressToScript 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.

Good idea, it's common way to determine if isMainnet by address prefix, in the mean time, when we need to translate Script to Address, we still need to get a isMainnet env from somewhere else, so I try to solve the two convertion problem together, maybe more common way is to call config.initializeConfig(isMainnet ? LINA : AGGRON4) when network changes? @homura

Copy link
Collaborator

@homura homura Jun 30, 2023

Choose a reason for hiding this comment

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

Can we move the address.startsWith('ckb') logic to helpers.addressToScript function?

OK, we'll implement it in the Lumos

outputType: '',
})
witnessEntry.witness = bytes.hexify(
blockchain.WitnessArgs.pack({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refer to https://github.com/nervosnetwork/neuron/pull/2734/files#diff-7703c04c04ff2f19a47617fbb9bd430893f7cb702bb9ef8185998fd2cdcb259dR73,
blockchain.WitnessArgs.pack can not accept spaces as inputType or outputType parameters, so should we change to

witnessEntry.witnessArgs.inputType ?? undefined

// input_type: undefined
// output_type: undefined
// }
expect(result).toEqual(HexUtils.byteLength('0x1400000010000000100000001400000000000000') + 4 + 4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may cause here https://github.com/ckb-js/lumos/blob/develop/packages/codec/src/blockchain.ts#L64-L66 Lumos set all fields as option. But ckb-sdk-js does not set fields as option

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel confused with https://github.com/ckb-js/lumos/blob/develop/packages/codec/src/blockchain.ts#L64-L66.
When I call bytes.hexify(base.blockchain.WitnessArgs.pack({ lock: '0x', input_type: '0x', output_type: '0x'})), it will output 0x1400000010000000140000001400000000000000, why input_type and output_type set empty, but lock set '0x00000000'.

Copy link
Contributor Author

@zhangyouxin zhangyouxin Jun 30, 2023

Choose a reason for hiding this comment

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

here '0x00000000' is args, check the test case setup:

const witnessArgs = new WitnessArgs('', '0x', '')

@@ -1158,7 +1161,8 @@ describe('TransactionGenerator', () => {
feeRate,
'0'
)
tx.witnesses[0] = serializeWitnessArgs(WitnessArgs.emptyLock().toSDK())
// tx.witnesses[0] = bytes.hexify(blockchain.WitnessArgs.pack(WitnessArgs.emptyLock().toSDK())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

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.

Refactor ckb-sdk-utils by lumos
4 participants