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

fix: signTransaction updated to work with latest v5 SDK #830

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

d4mr
Copy link
Member

@d4mr d4mr commented Jan 15, 2025

PR-Codex overview

This PR focuses on enhancing the sign-transaction functionality by incorporating the thirdwebClient, updating the transaction handling logic, and utilizing the prepareTransaction and toSerializableTransaction methods for improved transaction preparation and serialization.

Detailed summary

  • Replaced toTransactionType import with thirdwebClient.
  • Added imports for prepareTransaction, toSerializableTransaction, and getChain.
  • Updated chainId definition to be of type Integer.
  • Modified request handling to destructure chainId, nonce, and transaction.
  • Introduced chain variable using getChain.
  • Updated account retrieval to use dynamic chainId.
  • Created prepareTransactionOptions object with updated transaction handling.
  • Used prepareTransaction and toSerializableTransaction for transaction preparation and serialization.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

zeet-co bot commented Jan 15, 2025

We're building your pull request over on Zeet.
Click me for more info about your build and deployment.
Once built, this branch can be tested at: https://thirdweb-engine-ad8x-pb-fix-sign--e273c9.engine-aws-usw2.zeet.app before merging 😉

@@ -21,7 +25,7 @@ const requestBodySchema = Type.Object({
gasPrice: Type.Optional(Type.String()),
data: Type.Optional(Type.String()),
value: Type.Optional(Type.String()),
chainId: Type.Optional(Type.Integer()),
Copy link
Member

Choose a reason for hiding this comment

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

Is this gonna break existing users calling without a chain id?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, open to any way to support pre-eip155 signatures as well. I couldn't find a way with the current SDK.

Copy link
Member

Choose a reason for hiding this comment

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

Tbh signing a tx without a chain id is kinda sus. And I guess it will only break ppl when they upgrade?

Maybe we make it really clear in the release notes

@d4mr
Copy link
Member Author

d4mr commented Jan 15, 2025

A lot of this gets much better with zod schemas, so I have not invested much into make the validation super air tight here (updating to hexSchema etc)

@d4mr d4mr merged commit dd2d8e2 into main Jan 15, 2025
6 checks passed
@d4mr d4mr deleted the pb/fix-sign-transaction branch January 15, 2025 23:28
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