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

chore: Migrate transfer endpoints to v5 SDK #732

Merged
merged 4 commits into from
Oct 16, 2024
Merged

Conversation

arcoraven
Copy link
Contributor

@arcoraven arcoraven commented Oct 16, 2024

Maps the following routes to the v5 SDK methods:

  • erc20/transfer -> transfer
  • erc20/transferFrom -> transferFrom
  • erc721/transfer -> transferFrom
  • erc721/transferFrom -> transferFrom
  • erc1155/transfer -> safeTransferFrom
  • erc1155/transferFrom -> safeTransferFrom

PR-Codex overview

This PR focuses on enhancing the functionality and testing of ERC20, ERC721, and ERC1155 token transfers, along with improving schema definitions and utility functions. It introduces new test cases and refines existing code for better clarity and usability.

Detailed summary

  • Added TEST_ACCOUNT_B to test/e2e/utils/wallets.ts.
  • Introduced HexSchema in src/server/schemas/address.ts.
  • Updated example values in comments for amount in sdk/src/services/BackendWalletService.ts.
  • Changed import for Address in test/e2e/tests/setup.ts.
  • Removed console logs and improved assertions in test/e2e/tests/write.test.ts.
  • Refined parameter descriptions in various services (ERC20, ERC721, ERC1155).
  • Added data parameter to transfer functions in ERC1155.
  • Introduced getEngineBackendWalletB function in test/e2e/utils/engine.ts.
  • Updated contract interaction methods to use thirdweb client in multiple files.
  • Enhanced transfer tests for ERC20, ERC721, and ERC1155 with new assertions and setup logic.

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

examples: ["50000000000"],
});

export const NumberStringSchema = Type.RegExp(/^\d+$/, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useful for any generic non-decimal number input like tokenId or amount that must be a whole number (e.g. transferring NFTs).

Comment on lines +101 to +102
fromAddress: getChecksumAddress(walletAddress),
toAddress: getChecksumAddress(contractAddress),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making sure the from/to here is always backend wallet -> contract (as opposed to above when building the transaction where from/to are the sender/recipient addresses).

to: getChecksumAddress(to),
tokenId: BigInt(tokenId),
value: BigInt(amount),
data: "0x",
Copy link
Member

Choose a reason for hiding this comment

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

I'd expose data as an optional body param

to: getChecksumAddress(to),
tokenId: BigInt(tokenId),
value: BigInt(amount),
data: "0x",
Copy link
Member

Choose a reason for hiding this comment

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

expose data here too, and all the other routes

simulateTx,
extension: "erc1155",
idempotencyKey,
const transaction = safeTransferFrom({
Copy link
Member

Choose a reason for hiding this comment

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

safeTransferFrom can fail if the recipient is a contract that does not implement the onReceived hook. This is generally desirable behaviour, but could fail transfers to smart accounts that do not implement this method. Most smart accounts do implement this though, including ours I think, but would be best to confirm once.


return getAddress(newWallet.result.walletAddress);
}
const { result } = await engine.backendWallet.import({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Importing the same wallet into Engine is fine (it will dedupe)

@arcoraven arcoraven merged commit 410ba03 into main Oct 16, 2024
5 checks passed
@arcoraven arcoraven deleted the ph/moveTransfersToV5 branch October 16, 2024 18:58
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