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.

Sign up for GitHub

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

Jump to bottom

[token-js] : Support for UiAmountToAmount and AmountToUiAmount instructions #3345

Merged
merged 5 commits into from
Aug 24, 2022
Merged

[token-js] : Support for UiAmountToAmount and AmountToUiAmount instructions #3345

merged 5 commits into from
Aug 24, 2022

Conversation

atharmohammad
Copy link
Contributor

@atharmohammad atharmohammad commented Jul 14, 2022 •
edited
Loading

This PR adds :
1). support for amount to uiamount instruction
2). support for uiamount to amount instruction
3). upgrades solana/web3 from 1.41.0 to 1.47.4
4). e2e test for instructions

fixes #3264

cc : @joncinque

@mergify mergify bot added the community Community contribution label Jul 14, 2022
atharmohammad
atharmohammad commented Jul 14, 2022
View reviewed changes
token/js/package.json Outdated Show resolved Hide resolved
atharmohammad
atharmohammad commented Jul 14, 2022
View reviewed changes
token/js/package.json Outdated Show resolved Hide resolved
joncinque
joncinque reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This is in pretty good shape already! Just some notes and questions to make this ready. Also, be sure to run yarn lint or yarn lint:fix to get the formatting standard and fix the lint errors

token/js/test/e2e-2022/Amount.test.ts Outdated Show resolved Hide resolved
token/js/test/e2e-2022/Amount.test.ts Outdated Show resolved Hide resolved
token/js/test/e2e-2022/Amount.test.ts Outdated Show resolved Hide resolved
token/js/src/instructions/uiAmountToAmount.ts Outdated Show resolved Hide resolved
token/js/src/instructions/uiAmountToAmount.ts Outdated Show resolved Hide resolved
token/js/src/actions/uiAmountToAmount.ts Outdated Show resolved Hide resolved
token/js/src/actions/uiAmountToAmount.ts Outdated Show resolved Hide resolved
token/js/src/actions/uiAmountToAmount.ts Outdated Show resolved Hide resolved
token/js/src/actions/amountToUiAmount.ts Outdated Show resolved Hide resolved
token/js/src/actions/amountToUiAmount.ts Outdated Show resolved Hide resolved
@atharmohammad atharmohammad force-pushed the js-support-instructions branch 3 times, most recently from 6cbfdad to fa4551e Compare July 22, 2022 12:27
atharmohammad
atharmohammad commented Jul 22, 2022
View reviewed changes
token/js/src/instructions/uiAmountToAmount.ts
if (!instruction.programId.equals(programId)) throw new TokenInvalidInstructionProgramError();
const uiAmountToAmountInstructionData = struct<UiAmountToAmountInstructionData>([
u8('instruction'),
blob(instruction.data.length - 1, 'amount'),
Copy link
Contributor Author

@atharmohammad atharmohammad Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since length would be depending on the size of amount string , for decoding I assigned it data.length -1 , since u8 instruction would be taking 1 size. I checked that it is decoding correctly, is it fine or is there some better way for this

atharmohammad
atharmohammad commented Jul 22, 2022
View reviewed changes
token/js/src/instructions/uiAmountToAmount.ts
}: TransactionInstruction): DecodedUiAmountToAmountInstructionUnchecked {
const uiAmountToAmountInstructionData = struct<UiAmountToAmountInstructionData>([
u8('instruction'),
blob(data.length - 1, 'amount'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

@atharmohammad atharmohammad marked this pull request as ready for review July 22, 2022 13:13
@atharmohammad
Copy link
Contributor Author

@joncinque is the test failing because of upgrading @solana/web3.js ?

@joncinque
Copy link
Contributor

@atharmohammad Ah yeah, that's an annoying bug -- since two packages depend on @solana/web3.js, when you update, you'll have two separate entries in yarn.lock. You need to combine those to both resolve to the newer web3.js.

I should have noticed it earlier, but you'll also need to commit the change to yarn.lock

@atharmohammad atharmohammad marked this pull request as draft July 22, 2022 20:01
@atharmohammad atharmohammad force-pushed the js-support-instructions branch from fa4551e to c26fc79 Compare July 22, 2022 20:27
@atharmohammad
Copy link
Contributor Author

@joncinque So i have resolved web3 conflict issue, now i think test are failing because solana version for ci is v1.10.29 whereas for returnData we need version v1.11.4 so should i change solana version in ci ?

@joncinque
Copy link
Contributor

@atharmohammad you're correct -- I thought we had backported those changes to 1.10, but it isn't the case. We'll upgrade SPL to 1.11 once the release branch gets branched off of master, so not just yet. Which means we'll need to hold off on this a little bit longer.

@atharmohammad atharmohammad marked this pull request as ready for review July 26, 2022 18:08
@2501babe
Copy link
Member

@atharmohammad master is now on 1.11.6

@atharmohammad atharmohammad force-pushed the js-support-instructions branch from 3105033 to 75962d5 Compare August 21, 2022 09:08
joncinque
joncinque reviewed Aug 22, 2022
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, just two little things, then we can get this merged!

token/js/src/actions/uiAmountToAmount.ts Show resolved Hide resolved
token/js/test/e2e-2022/amount.test.ts Outdated Show resolved Hide resolved
@jordaaash
Copy link
Contributor

The resolved conflicts in this PR undo a bunch of the changes from #3508. This will need to be rebased against the current master.

@atharmohammad atharmohammad force-pushed the js-support-instructions branch from bfdbc71 to c39875a Compare August 23, 2022 19:00
@atharmohammad atharmohammad force-pushed the js-support-instructions branch from c39875a to d232e55 Compare August 23, 2022 19:35
@joncinque
Copy link
Contributor

@jordansexton was the rebase done correctly? Because otherwise this is ready to go from my side

jordaaash
jordaaash approved these changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

Thanks for rebasing! I added a commit to fix the .js file extensions in the imports and exports in the new code, and organize the imports. I just have one question about the web3.js version.

token/js/package.json Show resolved Hide resolved
@jordaaash jordaaash merged commit 2fe0ce6 into solana-labs:master Aug 24, 2022
@jordaaash
Copy link
Contributor

I'll release to npm tomorrow! Thanks for this!

@jordaaash
Copy link
Contributor

Published: @solana/spl-token@0.3.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordaaash jordaaash jordaaash approved these changes

@joncinque joncinque Awaiting requested review from joncinque

Assignees
No one assigned
Labels
community Community contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

token-js: Support UiAmountToAmount and AmountToUiAmount instructions
4 participants
@atharmohammad @joncinque @2501babe @jordaaash