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: send tokens #30

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat: send tokens #30

wants to merge 12 commits into from

Conversation

andreabadesso
Copy link
Collaborator

@andreabadesso andreabadesso commented Feb 12, 2025

Acceptance Criteria

  • We should be able to process the sendTransaction RPC request properly
  • We should upgrade the wallet-lib to the latest version

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged
  • Make sure either the unit tests and/or the QA tests are capable of testing the new features
  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@andreabadesso andreabadesso self-assigned this Feb 12, 2025
@andreabadesso andreabadesso added the enhancement New feature or request label Feb 12, 2025
@andreabadesso andreabadesso changed the base branch from master to feat/parameter-validation February 12, 2025 13:36
Copy link

@raul-oliveira raul-oliveira left a comment

Choose a reason for hiding this comment

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

NIT: change the pincode string to a more meaningful variable

const sendTransaction = await wallet.sendManyOutputsSendTransaction(sendTransactionOutputs, {
inputs: params.inputs || [],
changeAddress: params.changeAddress,
pinCode: '111111',

Choose a reason for hiding this comment

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

Magic string here. Assign a variable that holds the meaning of it.

IE stubPinCode or dumbPinCode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! Thanks

@andreabadesso andreabadesso changed the base branch from feat/parameter-validation to master March 6, 2025 17:23
@andreabadesso andreabadesso changed the base branch from master to feat/parameter-validation March 6, 2025 17:30
@andreabadesso andreabadesso changed the base branch from feat/parameter-validation to master March 6, 2025 17:34
network: z.string().min(1),
outputs: z.array(z.object({
address: z.string().optional(),
value: z.string().transform(val => BigInt(val)),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it optional when it's a data output?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, changed it to optional and added a better refine method to make it required only when not data output

Comment on lines 95 to 98
if (typedOutput.type === 'data') {
(typedOutput as unknown as { value: bigint }).value = BigInt(1);
typedOutput.token = '00';
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this? I think the lib method already handles it in case of a data output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually do need it because prepareTxData doesn't add the 1 HTR to data outputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress (Done)
Development

Successfully merging this pull request may close these issues.

3 participants