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

added support for txn >5kB #51

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

added support for txn >5kB #51

wants to merge 4 commits into from

Conversation

Suraj-Tiwari
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023

🦋 Changeset detected

Latest commit: 6de6bca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@cypherock/sdk-app-btc Patch
@cypherock/sdk-app-evm Patch
browser-test Patch
nodejs-test Patch

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

Comment on lines +23 to +26
assert(
params.txn.inputs.length === params.txn.rawTxn.length,
'txn.rawTxn should not be same length as txn.inputs',
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert and the error message is conflicting

Comment on lines +6 to +7
added support for txn >5kB
sign txn proto, test cases and flow update
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changeset should be in a single line

scriptPubKey: hexToUint8Array(input.scriptPubKey),
value: input.value,
sequence: input.sequence ?? signTxnDefaultParams.input.sequence,
chainIndex: input.chainIndex,
changeIndex: input.chainIndex,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the params to changeIndex as well

Suggested change
changeIndex: input.chainIndex,
changeIndex: input.changeIndex,

@@ -122,7 +120,7 @@ export const signTxn = async (
const { signature } = await helper.waitForResult();
assertOrThrowInvalidResult(signature);

signatures.push(uint8ArrayToHex(signature.signature));
signatures.push(uint8ArrayToHex(signature.unlockingScript));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return the signedTxn as well, check eth sign txn

outputs: ISignTxnOutput[];
inputs: ISignTxnInputData[];
outputs: ISignTxnOutputData[];
rawTxn: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this optional and fetch it from server when it's not provided.

Comment on lines +69 to +94

private static splitIntoChunks(txn: Uint8Array): Uint8Array[] {
const chunks: Uint8Array[] = [];
const totalChunks = Math.ceil(txn.length / OperationHelper.CHUNK_SIZE);

for (let i = 0; i < totalChunks; i += 1) {
const chunk = txn.slice(
i * OperationHelper.CHUNK_SIZE,
i * OperationHelper.CHUNK_SIZE + OperationHelper.CHUNK_SIZE,
);
chunks.push(chunk);
}

return chunks;
}

public async sendInChunks<
RK extends keyof Exclude<Result[R], null | undefined>,
QK extends keyof Exclude<Query[Q], null | undefined>,
>(data: Uint8Array, queryKey: QK, resultKey: RK) {
const chunks = OperationHelper.splitIntoChunks(data);
let remainingSize = data.length;

for (let i = 0; i < chunks.length; i += 1) {
const chunk = chunks[i];
remainingSize -= chunk.length;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to send in chunks in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was part of latest proto, it's split between 2 commits, this commit 8980e19419f43e7cd64a44edac81cc949cbee032 only contains old proto changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can open another pr from just that commit if we need to skip chink changes.

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

Successfully merging this pull request may close these issues.

2 participants