-
Notifications
You must be signed in to change notification settings - Fork 44
feat: Transactions package #146
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
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| const transaction = { | ||
| to: params.token as `0x${string}`, | ||
| data: tokenContract.write.transfer.toString(), // This needs proper encoding | ||
| // Note: This is a simplified version. In practice, you'd need to properly encode the function call |
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 ERC-20 token transfer implementation has an issue with the transaction data encoding. The current approach using tokenContract.write.transfer.toString() won't produce valid transaction data that can be executed on-chain.
For proper ERC-20 transfers, the transaction data needs to be encoded with the function signature and properly formatted parameters. Consider using viem's encodeFunctionData method instead:
import { encodeFunctionData } from 'viem'
// ...
const data = encodeFunctionData({
abi: ERC20_ABI,
functionName: 'transfer',
args: [params.to as `0x${string}`, value]
})
const transaction = {
to: params.token as `0x${string}`,
data
}This will ensure the transaction contains properly encoded calldata that the EVM can interpret correctly when executing the token transfer.
| const transaction = { | |
| to: params.token as `0x${string}`, | |
| data: tokenContract.write.transfer.toString(), // This needs proper encoding | |
| // Note: This is a simplified version. In practice, you'd need to properly encode the function call | |
| const transaction = { | |
| to: params.token as `0x${string}`, | |
| data: encodeFunctionData({ | |
| abi: ERC20_ABI, | |
| functionName: 'transfer', | |
| args: [params.to as `0x${string}`, value] | |
| }) | |
| } |
Spotted by Diamond
Is this helpful? React π or π to let us know.
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.
| const amount = typeof params.amount === 'string' | ||
| ? Math.floor(parseFloat(params.amount) * 1_000_000_000) | ||
| : typeof params.amount === 'bigint' | ||
| ? Number(params.amount) | ||
| : Math.floor(params.amount * 1_000_000_000); |
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 conversion from bigint to Number in the SUI amount calculation risks precision loss for large values. This is particularly problematic for cryptocurrency transactions where exact amounts are critical. Consider preserving the full precision by using bigint arithmetic throughout:
const amount = typeof params.amount === 'string'
? BigInt(Math.floor(parseFloat(params.amount) * 1_000_000_000))
: typeof params.amount === 'bigint'
? params.amount
: BigInt(Math.floor(params.amount * 1_000_000_000));This approach maintains precision for large transaction amounts while still handling all the input types correctly.
| const amount = typeof params.amount === 'string' | |
| ? Math.floor(parseFloat(params.amount) * 1_000_000_000) | |
| : typeof params.amount === 'bigint' | |
| ? Number(params.amount) | |
| : Math.floor(params.amount * 1_000_000_000); | |
| const amount = typeof params.amount === 'string' | |
| ? BigInt(Math.floor(parseFloat(params.amount) * 1_000_000_000)) | |
| : typeof params.amount === 'bigint' | |
| ? params.amount | |
| : BigInt(Math.floor(params.amount * 1_000_000_000)); |
Spotted by Diamond
Is this helpful? React π or π to let us know.
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.
@rafinskipg BigInt is probably the best stable type to use everywhere if possible? Like, just get rid of the fractional part at step 1 and stay in BigInt through everything?
| import { SuiTransactionBuilderImpl } from "./builders/sui"; | ||
|
|
||
| // Export types | ||
| export type * from "./types"; |
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 syntax export type * from "./types"; is not valid TypeScript. TypeScript doesn't support exporting all types with the type keyword in this manner. To properly export all types and interfaces from the types file, change this to export * from "./types"; which will export both types and values.
| export type * from "./types"; | |
| export * from "./types"; |
Spotted by Diamond
Is this helpful? React π or π to let us know.
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.
| { | ||
| files: ["tsup.config.ts"], | ||
| rules: { | ||
| "import/no-extraneous-dependencies": "off", |
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.
Do we strictly need to be turning off this rule globally?
| const transaction = { | ||
| to: params.token as `0x${string}`, | ||
| data: tokenContract.write.transfer.toString(), // This needs proper encoding | ||
| // Note: This is a simplified version. In practice, you'd need to properly encode the function call |
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.
| import { SuiTransactionBuilderImpl } from "./builders/sui"; | ||
|
|
||
| // Export types | ||
| export type * from "./types"; |
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.
|
|
||
| // Convert amount to MIST (1 SUI = 1,000,000,000 MIST) | ||
| const amount = typeof params.amount === 'string' | ||
| ? Math.floor(parseFloat(params.amount) * 1_000_000_000) |
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.
could make these methods generic with some baseTokenToAmount or whatever if you set up a lookup of MIST_PER_SUI / LAMPORTS_PER_SOL etc
| const amount = typeof params.amount === 'string' | ||
| ? Math.floor(parseFloat(params.amount) * 1_000_000_000) | ||
| : typeof params.amount === 'bigint' | ||
| ? Number(params.amount) | ||
| : Math.floor(params.amount * 1_000_000_000); |
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.
@rafinskipg BigInt is probably the best stable type to use everywhere if possible? Like, just get rid of the fractional part at step 1 and stay in BigInt through everything?
Adds a new transactions package to simplify transaction creation
π§ Core Features
β‘ Usage Examples