Refactor: replace 'HexUtils' with lumos functionalities

[zhangyouxin]

Description

This PR replaces some hexutil functions with @ckb-lumos/code and @ckb-lumos/bi, they are more tested and reliable.

Ref: Magickbase/neuron-public-issues#187

[github-actions]

Packaging for test is done in 5250939911 for commit 6b9637d .

[github-actions]

Packaging for test is done in 5250945890 for commit 6b68024 .

[github-actions]

Packaging for test is done in 5251157001 for commit 8bdb537 .

[homura]

There are some removePrefix we can refactor with bytes.hexify(bytes.concat(hash1, hash2)), after doing this, we can move the addPrefix and removePrefix to ledger module, this can reduce the complexity of the project by eliminating one module

[zhangyouxin]

There are some removePrefix we can refactor with bytes.hexify(bytes.concat(hash1, hash2)), after doing this, we can move the addPrefix and removePrefix to ledger module, this can reduce the complexity of the project by eliminating one module

Good advice about the removePrefix you mentioned here, but there are some other usages like findSignPathForCheque that may cause bug if removePrefix is moved to ledger module, anyway, I'm diving into the business logic here and get rid of removePrefix safely.

[homura]

There are some removePrefix we can refactor with bytes.hexify(bytes.concat(hash1, hash2)), after doing this, we can move the addPrefix and removePrefix to ledger module, this can reduce the complexity of the project by eliminating one module

Good advice about the removePrefix you mentioned here, but there are some other usages like findSignPathForCheque that may cause bug if removePrefix is moved to ledger module, anyway, I'm diving into the business logic here and get rid of removePrefix safely.

The findSignPathForCheque can also be refactored with bytes.equal

const receiverLockHash = bytify(chequeLockArgs).slice(0, 20)
const senderLockHash = bytify(chequeLockArgs).slice(20)

infos.find((info) => {
  const target = bytify(SystemScriptInfo.generateSecpScript(info.blake160).computeHash()).slice(0, 20);
  return equal(target, senderLockHash) || equal(target, receiverLockHash)
})

[zhangyouxin]

This is comprehensive but it's assuming that the input param chequeLockArgs starts with 0x, if not, the byte.bitify function will throw an error. we need to be careful here

[zhangyouxin]

There are some removePrefix we can refactor with bytes.hexify(bytes.concat(hash1, hash2)), after doing this, we can move the addPrefix and removePrefix to ledger module, this can reduce the complexity of the project by eliminating one module

Good advice about the removePrefix you mentioned here, but there are some other usages like findSignPathForCheque that may cause bug if removePrefix is moved to ledger module, anyway, I'm diving into the business logic here and get rid of removePrefix safely.

The findSignPathForCheque can also be refactored with bytes.equal

const receiverLockHash = bytify(chequeLockArgs).slice(0, 20)
const senderLockHash = bytify(chequeLockArgs).slice(20)

infos.find((info) => {
  const target = bytify(SystemScriptInfo.generateSecpScript(info.blake160).computeHash()).slice(0, 20);
  return equal(target, senderLockHash) || equal(target, receiverLockHash)
})

Seems not easy to do so, as described in previous comment, the input param chequeLockArgs and the recieverLockHash and senderLockHash when calling generateChequeScript are not really HexString, many test cases use this pattern '0'.repeat(byteLength * 2), I've tried to change them to bytes.hexify(Buffer.alloc(20)), but still causing weird errors in tests. Also, we are not very sure about whether the input params are HexString in real runtime env.

[zhangyouxin]

in this commit I have removed the HexUtil module and changed some tests, please take a look @homura @yanguoyu

[github-actions]

Packaging for test is done in 5255925402 for commit 16a7700 .

[github-actions]

Packaging for test is done in 5256989537 for commit afb1839 .

[github-actions]

Packaging for test is done in 5297858260 for commit 0b29647 .

[github-actions]

Packaging for test is done in 5301095593 for commit 66c19b9 .

[github-actions]

Packaging for test is done in 5307543220 for commit 72741e4 .

[zhangyouxin]

Sorry I didn't notice that all commits should be "Verified", so I squashed all commits and signed it with my GPG key, then the squashed commit needs to be force pushed, will sign every commit in future.

[github-actions]

Packaging for test is done in 5307915541 for commit f601d45 .

[github-actions]

Packaging for test is done in 5307929099 for commit e07c29d .

[homura]

The conflicts are waiting for resolved

[github-actions]

Packaging for test is done in 5369142913 for commit f1bfe29 .

[github-actions]

Packaging for test is done in 5369157237 for commit 3605640 .

[github-actions]

Packaging for test is done in 5369463207 for commit 65eb36b .

[github-actions]

Packaging for test is done in 5373006588 for commit 6b724e1 .

[FrederLu]

@zhangyouxin
What does this pr modify? What does QA need to verify? Or verify what kind of scene.

[zhangyouxin]

@zhangyouxin What does this pr modify? What does QA need to verify? Or verify what kind of scene.

This PR has changed some low level function dependencies, mostly used in the packages/neuron-wallet package, though it's not a big change, it may affect many functionalities, such as Cell Service, Transaction Service, QA may need adequate regression test for syncing, show balance, send transaction and so on.

[github-actions]

Packaging for test is done in 5384478240 for commit 4da3f2b .

[littleLip520]

use multisig address do transaction to normal address occur an erorr.

@zhangyouxin

main.log

[littleLip520]

after using asset account do transaction. seems transaction detail did't match trans amount. @zhangyouxin

add hash
0x521c210c81616f95bfa6ac63a427d5145c59ecdf4c4c79e6dcea9fe51fb18bb7

[homura]

@zhangyouxin any update of this ?

[zhangyouxin]

/package
Packaging for test is done in 5552021555. @zhangyouxin

[zhangyouxin]

use multisig address do transaction to normal address occur an erorr.

@zhangyouxin

main.log

I have merged develop branch, tested the feature on local machine and didn't meet the issue, is there more detail of condition to trigger the two issues?

btw I found another issue which may not be introduced by this PR: Magickbase/neuron-public-issues#222

[zhangyouxin]

after using asset account do transaction. seems transaction detail did't match trans amount. @zhangyouxin

add hash 0x521c210c81616f95bfa6ac63a427d5145c59ecdf4c4c79e6dcea9fe51fb18bb7

After confirmation with @FrederLu , we found that in this transaction, the user business is extracting 9999 sUDT from a cell with ACP lock. So both sudt receive amount and CKB change amount are correct value here.