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 ckb-sdk-utils by lumos #201

Closed
Tracked by #177
homura opened this issue Jun 27, 2023 · 6 comments · Fixed by nervosnetwork/neuron#2817
Closed
Tracked by #177

Refactor ckb-sdk-utils by lumos #201

homura opened this issue Jun 27, 2023 · 6 comments · Fixed by nervosnetwork/neuron#2817

Comments

@homura
Copy link

homura commented Jun 27, 2023

https://lumos-website.vercel.app/migrations/migrate-form-ckb-sdk-utils

@homura homura mentioned this issue Jun 27, 2023
35 tasks
@homura homura changed the title utils Refactor ckb-sdk-utils by lumos Jun 27, 2023
@zhangyouxin
Copy link

nervosnetwork/neuron#2734 was closed due to two major reasons:

  1. there are too many isMainnet ? LINA : AGGRON4 expressions in the refator
  2. the serialization of ckb-sdk-util and lumos don't receive the same parameter type of blank value, utils uses null and lumos uses undefined for blank value, they are not compatible, so the refator brings complexity by transforming objects that needs to be serilized

For the first problem, we could add a new manager for network switching.

For the second problem, we need to transform null to undefined,this util can either be provided by lumos or implement in Neuron. An example:

// witness args before transform
const witnessArgs = {
  lock: `0x${'00'.repeat(65)}`,
  inputType: null,
  outputType: null,
}

// witness args after transform
const witnessArgs = {
  lock: `0x${'00'.repeat(65)}`,
  inputType: undefined,
  outputType: undefined,
}

In that case, the most certain parts are:

  • scriptToHash(script) can be replaced with utils.computeScriptHash(script)
  • src/models/blake2b.ts can be replaced with lumos

I think maybe we can implement the certain part first

@homura
Copy link
Author

homura commented Aug 2, 2023

For the first problem, we could add a new manager for network switching.

You mentioned manager here sound like a stateful?

We need to transform null to undefined

How many file changes will this refactoring cause?

@zhangyouxin
Copy link

You mentioned manager here sound like a stateful?

I was not very sure about this.

  • option 1: It could be a stateful class and exists as a singleton instance in Neuron runtime env, so that the instance can be accessed and provides a getNetworkType method.
  • option 2: The network type can be persisted in electron-store-like data store, and we provide a set of util functions to CRUD the current network type

I thouht maybe we can use option 2, and after reading about current code. I find that Neuron is already using option 1:

So now that I can just utilize this and add a LumosConfigService service similar to network service, it is a singleton class providing a getLumosConfig public method.

How many file changes will this refactoring cause?

I think it could be one file, providing util functions to transform Script, WitnessArgs, RawTransaction, CellDep, Transaction, OutPoint, Block, CellInput, CellOutput, Header.(need to test all these transformers).
And in real business code, we just need to change ckb-sdk-utils/serilize(obj) to lumos-codec/pack(transform(obj))

@homura
Copy link
Author

homura commented Aug 3, 2023

So now that I can just utilize this and add a LumosConfigService service similar to network service, it is a singleton class providing a getLumosConfig public method.

A stateful manager may cause other problems, so maybe we can keep the API the same as before

type scriptToAddress = (script:Script, isMainnet?:boolean) => string;

And remove the address conversions that were dependent on the ckb-sdk-js

I think it could be one file

It's LGTM

@yanguoyu
Copy link

The @nervosnetwork/ckb-sdk-utils package was not removed from the UI package. Will we continue to remove @nervosnetwork/ckb-sdk-utils? https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-ui/package.json#L53-L54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants