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

feat: add solana token 2022 support #3968

Merged
merged 16 commits into from
Aug 16, 2024
Merged

feat: add solana token 2022 support #3968

merged 16 commits into from
Aug 16, 2024

Conversation

weixuefeng
Copy link
Contributor

fix: #3934

satoshiotomakan
satoshiotomakan requested changes Aug 5, 2024
View reviewed changes
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Hi @weixuefeng, thank you for opening the PR.
Please also fix rustfmt and(?) clippy warnings

rust/chains/tw_solana/src/modules/compiled_keys.rs Outdated
.iter()
.filter_map(|key| filter(key, true, true))
.collect();
let readonly_signer_keys: Vec<SolanaAddress> = ordered_keys
writable_signer_keys.sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please revert the keys sorting? Even though, they're sorted in official Solana lib, but in WalletCore I disabled it explicitly to save backward compatibility to have the same transaction signature after the Rust transition.
Please note that the keys are not required to be sorted to be valid transaction

rust/chains/tw_solana/src/modules/compiled_keys.rs Outdated
.iter()
.filter_map(|key| filter(key, true, false))
.collect();
let writable_non_signer_keys: Vec<SolanaAddress> = ordered_keys
readonly_signer_keys.sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

rust/chains/tw_solana/src/modules/compiled_keys.rs Outdated
.iter()
.filter_map(|key| filter(key, false, true))
.collect();
writable_non_signer_keys.sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

rust/tw_any_coin/tests/chains/solana/solana_sign.rs Outdated
Comment on lines 558 to 559
is_token_2022: false,
..Proto::CreateAndTransferToken::default()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can ignore is_token_2022 field here as ..Proto::CreateAndTransferToken::default() leaves it default (false) value. Same in other places

@satoshiotomakan
Copy link
Collaborator

@weixuefeng I can push the fix commit if you're ok, just let me know

@weixuefeng
Copy link
Contributor Author

@weixuefeng I can push the fix commit if you're ok, just let me know

updated. @satoshiotomakan

@satoshiotomakan
Copy link
Collaborator

I can't push fix commits, so please run cargo fmt at the rust directory. Please also make sure all tests pass, and cargo clippy do not produce warnings

@satoshiotomakan
Copy link
Collaborator

Hi @weixuefeng, did you have a chance to fix the formatting by running cargo fmt and cargo clippy?

@weixuefeng
Copy link
Contributor Author

Hi @weixuefeng, did you have a chance to fix the formatting by running cargo fmt and cargo clippy?

only fix cargo fmt and cargo clippy. but rust test have some error. i don't know how can i generate signature. and the preImageHash for transaction is different. i have not resolve it.

@satoshiotomakan
Copy link
Collaborator

Hi @weixuefeng, do you mean you can fix test_solana_sign_create_and_transfer_token_2022 and test_solana_sign_transfer_token_2022 tests after removing the keys reordering? You can regenerate and rebroadcast the transactions.
Other tests should pass since there is no breaking changes anymore if is_token_2022: false

@weixuefeng
Copy link
Contributor Author

Hi @weixuefeng, do you mean you can fix test_solana_sign_create_and_transfer_token_2022 and test_solana_sign_transfer_token_2022 tests after removing the keys reordering? You can regenerate and rebroadcast the transactions. Other tests should pass since there is no breaking changes anymore if is_token_2022: false

yes. removing the keys reordering can broadcast success on devnet.

but there have some problem. i have not found the problem about test_solana_compile_create_and_transfer_token_with_external_fee_payer testcase.
@satoshiotomakan

@0xh3rman
Copy link
Contributor

@weixuefeng @satoshiotomakan Maybe it's better to create an enum for token standard? not bool type or we can pass the token program as an argument in protobuf model cause it seems compatible with Token Program

@weixuefeng
Copy link
Contributor Author

@weixuefeng @satoshiotomakan Maybe it's better to create an enum for token standard? not bool type or we can pass the token program as an argument in protobuf model cause it seems compatible with Token Program

I think tokenProgramId is very slow to update, so I stick with bool value. How about refactoring if there are more programId in the future?
@0xh3rman

@weixuefeng
Copy link
Contributor Author

Please check update. @satoshiotomakan

satoshiotomakan
satoshiotomakan requested changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Hi @weixuefeng, I agree with @0xh3rman, could you please add an enum in Solana.proto. Please make a few small changes as well

rust/chains/tw_solana/src/modules/instruction_builder/token_instruction.rs Outdated Show resolved Hide resolved
rust/chains/tw_solana/src/modules/instruction_builder/token_instruction.rs Outdated Show resolved Hide resolved
0xh3rman
0xh3rman reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@0xh3rman 0xh3rman left a comment

Choose a reason for hiding this comment

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

LGTM

rust/chains/tw_solana/src/modules/message_builder.rs Outdated Show resolved Hide resolved
@weixuefeng @0xh3rman
refactor: format code
3ce2733 
Co-authored-by: 0xh3rman <119309671+0xh3rman@users.noreply.github.com>
@satoshiotomakan
Copy link
Collaborator

Hi @weixuefeng, could you please format the code using cargo fmt? Please also make sure cargo clippy doesn't produce warnings, and all tests pass. I'm sure there is no breaking changes, so test_solana_compile_create_and_transfer_token_with_external_fee_payer should also pass without problems

satoshiotomakan
satoshiotomakan approved these changes Aug 16, 2024
View reviewed changes
Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

LGTM

@satoshiotomakan satoshiotomakan merged commit 160a6e6 into trustwallet:master Aug 16, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xh3rman 0xh3rman 0xh3rman left review comments

@satoshiotomakan satoshiotomakan satoshiotomakan approved these changes

@Milerius Milerius Awaiting requested review from Milerius Milerius is a code owner

@ar-g ar-g Awaiting requested review from ar-g

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@weixuefeng @satoshiotomakan @0xh3rman