-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat(helpers): model helpers for ckb object #694
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #694 +/- ##
===========================================
+ Coverage 84.30% 86.89% +2.58%
===========================================
Files 121 131 +10
Lines 24574 25343 +769
Branches 2364 2744 +380
===========================================
+ Hits 20717 22021 +1304
+ Misses 3816 3281 -535
Partials 41 41
... and 43 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
I didn't get the use case of |
There are two primary purposes of this PR:
ConvenienceIn the past, if developers wanted to create a const cell: Cell = {
cellOutput: {
lock: {...},
capacity: '0x0'
},
data: '0x'
};
cell.cellOutput.capacity = minimalCapacityCompatible(cell).toHexString(); This process was heavy during transaction assembly. This PR introduces the concept of CellHelper.create({
lock: 'ckt1...', // ScriptLike
type: {
codeHash: randomBytes(32), // BytesLike is acceptable
hashType: 'type',
args: Buffer.from([...]) // BytesLike is acceptable
},
data: Uint128.pack(111) // BytesLike is acceptable
} /* CellLike */); RobustIn the past, developers had multiple ways to interact with CKB-related objects, such as comparing two if (
codeHash === anotherCodeHash &&
hashType === anotherHashType &&
args === anotherArgs
) {
// ...
} This approach may cause unexpected behaviors, such as when the hex format string is not the same case, e.g. |
It's pretty OOP, so why not const cell = new Cell({
codeHash: "hash"
})
// or
const cell = Cell.new({
codeHash: 'hash'
}) and make the class Cell {
static hash(cell) {
if (cell instanceof Cell) return cell.hash
return new Cell(cell).hash
}
static equals(a, b) {
const [c, d] = [a, b].map(i => i instanceof Cell ? i : new Cell(i))
return c.equals(d)
}
getter hash() {
return this._hash()
}
equals (cell) {
return this._equals(cell)
}
}
const cellHash = new Cell().hash
Cell.equals(a, b)
new Cell(a).equals(b) |
The const { injectCapacity } = common;
txSkeleton = await injectCapacity(txSkeleton, ...); instead of await txSkeleton.injectCapacity(...) To keep a more consistent code style, OOP in JavaScript may cause some unexpected behavior like this: class Named {
name = 'Alice';
sayHi() {
console.log('Hi, I am ' + this.name);
}
}
const named = new Named();
const { sayHi } = named;
sayHi(); // Cannot read properties of undefined... There were many object deconstructions used in Lumos or used with Lumos; therefore, a helper may be a more suitable for Lumos |
It's counterintuitive to me because it looks like OOP but works in FP way. Maybe I'm misled by the name How about defining it as a Or simply name it |
Indeed, I hadn't realized that the first first capitalized camel naming implied that the reference should be a class instead of a plain object
I'd prefer |
packages/helpers/README.md
Outdated
```javascript | ||
import { CellHelper } from "@ckb-lumos/helpers"; | ||
|
||
const cell = CellHelper.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CellHelper
should be cellHelper
Description
This PR introduced a new helper
ModelHelper
to provide some common functions for CKB-related objectsA Glance
ModelHelpers
Type of change
Please delete options that are not relevant.
Tested?