Refactor: remove models/script and repalce with lumos by zhangyouxin · Pull Request #2762 · nervosnetwork/neuron

zhangyouxin · 2023-07-06T07:24:31Z

Description

This PR removes most of model/chain/Script.ts in packages/neuron-wallet, and a subtask extracted from #2734

changed

refactored Script class
remove method fromObject
remove method toSDK
remove method computeHash
change calculateScriptBytesize to a pure function (maybe refactor to codec in future)

zhangyouxin · 2023-07-06T07:29:12Z

-  }
-
-  public calculateBytesize(): number {
-    return 1 + HexUtils.byteLength(this.codeHash) + HexUtils.byteLength(this.args)


Tried to replace with blockchain.Script.pack(script).bytelength, but that way the molecule encoding introduces 20 bytes of header.

Not sure why we need this logic here but just keep it, if this logic is used to calculate tx size, then it can be refactored to blockchain.Script.pack in the future.

This is not used to calculate tx size, it is used to calculate how many capacity needs to hold the cell.

zhangyouxin · 2023-07-06T07:31:43Z

  private multiSignOutPointInfo = new Map<string, OutPoint>()

+  public getDaoScriptHash(): Hash {
+    return utils.computeScriptHash(SystemScriptInfo.DAO_SCRIPT)


static DAO_SCRIPT_HASH = utils.computeScriptHash(DAO_SCRIPT) is not a good pattern and somehow doesn't work, it could be a constant in pure utility file, but here in a class just use a public method is ok.

the DAO_SCRIPT is static, why we should use a function to compute it instead of just export it as a static. Is there any context of static DAO_SCRIPT_HASH = utils.computeScriptHash(DAO_SCRIPT) is not a good pattern and somehow doesn't work

Can you help me be by fetching this branch on your local machine and change to static DAO_SCRIPT_HASH = utils.computeScriptHash(DAO_SCRIPT), I thought it should work, but it just doesn't, always prompting an error when i ran yarn test, didn't find a proper reason but class method works, so I just blamed it on static methods.

zhangyouxin · 2023-07-06T07:33:21Z

      capacity: nftCell.capacity,
      lock: outputLock,
-      type: nftCell.type ? new Script(nftCell.type.codeHash, nftCell.type.args, nftCell.type.hashType) : null,
+      type: nftCell.type ?? null,


The original code defaults to null, so just keep it the same

IMO, we can only set nftCell.type

zhangyouxin · 2023-07-06T07:37:50Z

  const addressInfo: Address = {
    address,
-    blake160: '0xfakeblake160',
+    blake160: `0x${'0'.repeat(40)}`,


This blake160 here will be used to calculate args then utils.computeScriptHash(script: Script), so that blake160 needs to be a valid HexString here, otherwise it won't build.

zhangyouxin · 2023-07-06T07:39:28Z

-      Script.fromObject({ codeHash: '', args: '', hashType: ScriptHashType.Type }),
-      Script.fromObject({ codeHash: '', args: '', hashType: ScriptHashType.Type }),
+      { codeHash: '0x' + '0'.repeat(64), args: '0x', hashType: 'type' },
+      { codeHash: '0x' + '0'.repeat(64), args: '0x', hashType: 'type' },


Same as previous, utils.computeScriptHash(script: Script) needs valid 'HexString's

zhangyouxin · 2023-07-06T07:40:23Z

-    const lockScript2 = new Script(codeHash, fakeArgs2, ScriptHashType.Type)
-    const lockScript3 = new Script(codeHash, fakeArgs3, ScriptHashType.Type)
+    const lockScript1: Script = {
+      args: '0x01',


Same as previous, utils.computeScriptHash(script: Script) needs valid 'HexString's

zhangyouxin · 2023-07-06T07:50:18Z

      })

      describe('with full address', () => {
-        it(`only full address, 43 capacity`, async () => {


This test case is meant to be unchanged, but it just fails anyway, I find that 43 CKBytes is not enough to send a transaction though, so it always throws out an error of capacity too small, I think it should be at least 61 ?

zhangyouxin · 2023-07-06T07:50:48Z

-            lock: Script.fromObject({
-              args: '',
+            lock: {
+              args: '0x',


Same as previous, utils.computeScriptHash(script: Script) needs valid 'HexString's

zhangyouxin · 2023-07-06T07:51:47Z

@@ -1,7 +1,7 @@
 import { In, MigrationInterface, QueryRunner } from "typeorm";


maybe I should not edit this migration file, but it does not build if I don't change it, how should it be?

I believe changing the type will not have a side effect.

homura · 2023-07-06T15:50:48Z

        ),
        codeHash: process.env.MAINNET_SUDT_SCRIPT_CODEHASH!,
-        hashType: process.env.MAINNET_SUDT_SCRIPT_HASHTYPE! as ScriptHashType,
+        hashType: process.env.MAINNET_SUDT_SCRIPT_HASHTYPE! as HashType,


process.env.MAINNET_SUDT_SCRIPT_HASHTYPE can be re-declared

homura · 2023-07-06T15:54:50Z

-  Data1 = 'data1',
-}
-
-export default class Script {


Is there any reason to remove the Script class? Neuron uses the class style model, maybe we can just keep using it

There is a big task of refactoring with lumos and there were many fromObject and toSDK calls in the codebase, with this PR the codes can be more simple and "lumosified", but this PR could be closed if it's not a necessary one.

homura · 2023-07-06T16:03:42Z

            {
              address: addressMeta.address,
-              lockHash: lockScript.computeHash(),
+              lockHash: utils.computeScriptHash(lockScript),


I think we can keep using the class model in Neuron to keep the code pattern

homura · 2023-07-06T16:06:08Z

        if (
-          previousOutput.type?.computeHash() === SystemScriptInfo.DAO_SCRIPT_HASH &&
+          previousOutput.type &&
+          utils.computeScriptHash(previousOutput.type) === SystemScriptInfo.getInstance().getDaoScriptHash() &&


Why we should refactor the SystemScriptInfo.DAO_SCRIPT_HASH here?

homura · 2023-07-06T16:11:16Z

  private multiSignOutPointInfo = new Map<string, OutPoint>()

+  public getDaoScriptHash(): Hash {
+    return utils.computeScriptHash(SystemScriptInfo.DAO_SCRIPT)


the DAO_SCRIPT is static, why we should use a function to compute it instead of just export it as a static. Is there any context of static DAO_SCRIPT_HASH = utils.computeScriptHash(DAO_SCRIPT) is not a good pattern and somehow doesn't work

yanguoyu · 2023-07-07T02:15:09Z

-    this.lockHash = lockHash || this.lock?.computeHash()
-
+    this.lockHash = lockHash
+    if (lock) {


Suggested change

if (lock) {

if (lock && !lockHash) {

yanguoyu · 2023-07-07T02:15:28Z

-    this.typeHash = typeHash || this.type?.computeHash()

+    this.typeHash = typeHash
+    if (type) {


Suggested change

if (type) {

if (type && !typeHash) {

yanguoyu · 2023-07-07T02:25:21Z

-          LUMOS_HASH_TYPE_MAP[cell.cellOutput.type.hashType] ?? ScriptHashType.Data
-        )
-      : null
+    const type = (cell.cellOutput.type as Script) || null


Maybe we can define LumosCell.cellOutput.type to Script

yanguoyu · 2023-07-07T02:28:16Z

-        cell.cellOutput.lock.args,
-        LUMOS_HASH_TYPE_MAP[cell.cellOutput.lock.hashType] ?? ScriptHashType.Data
-      ),
+      cell.cellOutput.lock as Script,


Maybe we can define LumosCell.cellOutput.lock to Script

yanguoyu · 2023-07-07T02:31:33Z

-  }
+// calculate bytelength of script, but without the header bytes of molecule encoding
+// TODO: refactor me with @ckb-lumos/codec
+export const calculateScriptBytesize = (script: Script): number => {


Is Bytesize a word? Maybe ByteSize?

yanguoyu · 2023-07-07T02:53:38Z

-  }
-
-  public calculateBytesize(): number {
-    return 1 + HexUtils.byteLength(this.codeHash) + HexUtils.byteLength(this.args)


This is not used to calculate tx size, it is used to calculate how many capacity needs to hold the cell.

yanguoyu · 2023-07-07T02:58:41Z

      capacity: nftCell.capacity,
      lock: outputLock,
-      type: nftCell.type ? new Script(nftCell.type.codeHash, nftCell.type.args, nftCell.type.hashType) : null,
+      type: nftCell.type ?? null,


IMO, we can only set nftCell.type

refactor: remove most of script.ts

09b6d1c

zhangyouxin commented Jul 6, 2023

View reviewed changes

fix: fix typos

6954b0b

zhangyouxin marked this pull request as ready for review July 6, 2023 08:13

zhangyouxin requested review from FrederLu, Keith-CY, WhiteMinds, alexsupa597, devchenyan, homura and yanguoyu July 6, 2023 08:22

homura reviewed Jul 6, 2023

View reviewed changes

yanguoyu reviewed Jul 7, 2023

View reviewed changes

zhangyouxin closed this Jul 19, 2023

		@@ -1,7 +1,7 @@
		import { In, MigrationInterface, QueryRunner } from "typeorm";

Uh oh!

Conversation

zhangyouxin commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

changed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhangyouxin commented Jul 6, 2023 •

edited

Loading