-
Notifications
You must be signed in to change notification settings - Fork 24
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: rewrite spore-sdk in ccc #43
Conversation
🦋 Changeset detectedLatest commit: 3cc27d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/spore/src/advanced.ts
Outdated
signer: ccc.Signer, | ||
tx: ccc.TransactionLike, | ||
cobuildActions: UnpackResult<typeof ActionVec>, | ||
feeRate: number = 1000, |
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.
The feeRate
should not be set by default. Developers / Users should set this dynamically to avoid on-chain attackers.
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.
ok, I'll leave comment to tell users 1000 is the minimum fee rate
packages/spore/src/advanced.ts
Outdated
import { ccc } from "@ckb-ccc/core"; | ||
import { bytes, UnpackResult } from "@ckb-lumos/codec"; | ||
import { Action, ActionVec, SporeAction, WitnessLayout } from "./codec"; | ||
import { buildProtoclScript, SporeScriptInfo } from "./predefined"; |
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.
For commonjs compatibility, please add .js
extension at the end of files path.
packages/spore/src/advanced.ts
Outdated
cobuildActions: UnpackResult<typeof ActionVec>, | ||
feeRate: number = 1000, | ||
): Promise<ccc.Transaction> { | ||
if (feeRate < 1000) { |
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.
This check should be done by the node / UI.
packages/spore/src/advanced.ts
Outdated
|
||
// change cell | ||
let txSkeleton = ccc.Transaction.from(tx); | ||
txSkeleton.addOutput({ lock }); |
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.
It's not necessary to add a change cell here. we can use completeFeeBy
.
packages/spore/src/advanced.ts
Outdated
sporeOutput: ccc.CellOutputLike, | ||
sporeData: ccc.BytesLike, | ||
): UnpackResult<typeof Action> { | ||
const sporeType = ccc.Script.from(sporeOutput.type!); |
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.
Please check if the type is null here instead of ignoring it. (And also below)
packages/spore/src/helper.ts
Outdated
...liveCell, | ||
}), | ||
); | ||
txSkeleton.witnesses.push("0x"); |
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.
Is it possible to use Signer.prepareTransaction
after collecting all inputs? Extra witnesses don't look necessary.
tx: ccc.TransactionLike, | ||
outputIndex: number, | ||
): ccc.Hex { | ||
const firstInput = tx.inputs ? tx.inputs[0] : void 0; |
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.
tx.inputs
always exist.
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.
inputs is nullable in TransactionLike
packages/spore/src/predefined.ts
Outdated
args, | ||
...scriptInfo[procotol], | ||
}); | ||
} else { |
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.
else
here is not needed because we returned before.
packages/spore/vitest.config.mts
Outdated
@@ -0,0 +1,17 @@ | |||
import { defineConfig } from "vitest/config"; |
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.
The test framework for CCC is jest. CCC is also built for backend developers.
pnpm-lock.yaml
Outdated
@@ -1,4 +1,4 @@ | |||
lockfileVersion: '9.0' | |||
lockfileVersion: '6.0' |
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.
Please upgrade your pnpm version.
@Hanssen0 I've solved all reported issues in comment, new commit has been committed, please repeat your review |
dc62d8a
to
7bfdaff
Compare
7bfdaff
to
2de265a
Compare
feat: rewrite spore-sdk in ccc
0338e69
to
2e849eb
Compare
Merged in #74 |
Description
spore-sdk
in ccc, only contain features for spore and clustertype-burn
andlock-proxy
as known scripts