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

feat: kernel batch transactions and gas estimation fixes #39

Merged
merged 22 commits into from
Jul 5, 2023

Conversation

whatmanathinks
Copy link
Contributor

No description provided.

);

estimates.verificationGasLimit =
(BigInt(estimates.verificationGasLimit) * 130n) / 100n;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moldy530

I didn't have accurate answer to your question about requiring 30% extra gas usage for kernel. Turns out, it was because i added an inaccurate dummy signature which leads to underestimation of gas. Fixed in this update

@whatmanathinks
Copy link
Contributor Author

Build will only pass once new version from main branch is released

@moldy530
Copy link
Collaborator

ah nice good catch! will let you know once I push an update

@moldy530
Copy link
Collaborator

@ManaOmlabs sorry for the delay here, was trying to automate deploys somewhat. That's all working now and the latest version should be published!

@whatmanathinks whatmanathinks changed the title Kernel Gas Estimation Fixes feat: kernel batch transactions and gas estimation fixes Jun 30, 2023
@whatmanathinks
Copy link
Contributor Author

@moldy530 ready to review

@@ -198,22 +196,31 @@ describe("Kernel Account Tests", () => {
// await expect(result).resolves.not.toThrowError();
// });

// it("sendUserOperation batch should execute properly", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ManaOmlabs , the aa infra on alchemy is now open to the public so it should be possible to make these tests pass here. However, @avasisht23 is working on splitting tests into integration vs unit tests so we'll make sure to port these over as integration tests since they require an alchemy api key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. I will be on lookout for the respective 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.

Handled

}
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make this @ts-ignore: <reason for ignore> also we shouldn't use @ts-ignore and instead use @ts-expect-error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

[
call.delegateCall ? 1 : 0,
call.target,
call.value || BigInt(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
call.value || BigInt(0),
call.value ?? BigInt(0),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@whatmanathinks
Copy link
Contributor Author

@moldy530 let me know if any more changes needed

Copy link
Collaborator

@moldy530 moldy530 left a comment

Choose a reason for hiding this comment

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

LGTM with some nits and comments on TS errors

Won't be online today much so can take a look again tmr

): Promise<`0x${string}`> {
// @ts-ignore
const multiSendData: `0x${string}` =
"0x" + _txs.map((tx) => encodeCall(tx)).join("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw you could use the concat util from viem and just do

concat(_txs.map(...))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

override async encodeBatchExecute(
_txs: BatchUserOperationCallData
): Promise<`0x${string}`> {
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

}
// @ts-expect-error: signWith6492 exists only on kernel account which is expected
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh you could fix this by either overriding the account param on the kernel provider so it has more specific typing or making the provider take a generic A extends BaseSmartContractAccount = BaseSmartContractAccount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

@whatmanathinks
Copy link
Contributor Author

@moldy530 everything handled :)

@moldy530 moldy530 merged commit f2a3d3d into alchemyplatform:main Jul 5, 2023
rthomare pushed a commit that referenced this pull request Oct 4, 2023
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