-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: add solana token 2022 support #3968
feat: add solana token 2022 support #3968
Conversation
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.
Hi @weixuefeng, thank you for opening the PR.
Please also fix rustfmt and(?) clippy warnings
.iter() | ||
.filter_map(|key| filter(key, true, true)) | ||
.collect(); | ||
let readonly_signer_keys: Vec<SolanaAddress> = ordered_keys | ||
writable_signer_keys.sort(); |
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 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
.iter() | ||
.filter_map(|key| filter(key, true, false)) | ||
.collect(); | ||
let writable_non_signer_keys: Vec<SolanaAddress> = ordered_keys | ||
readonly_signer_keys.sort(); |
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.
Same here
.iter() | ||
.filter_map(|key| filter(key, false, true)) | ||
.collect(); | ||
writable_non_signer_keys.sort(); |
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.
Same here
is_token_2022: false, | ||
..Proto::CreateAndTransferToken::default() |
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.
You can ignore is_token_2022
field here as ..Proto::CreateAndTransferToken::default()
leaves it default (false) value. Same in other places
@weixuefeng I can push the fix commit if you're ok, just let me know |
updated. @satoshiotomakan |
I can't push fix commits, so please run |
Hi @weixuefeng, did you have a chance to fix the formatting by running |
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. |
Hi @weixuefeng, do you mean you can fix |
yes. removing the keys reordering can broadcast success on devnet. but there have some problem. i have not found the problem about |
@weixuefeng @satoshiotomakan Maybe it's better to create an enum for token standard? not |
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? |
Please check update. @satoshiotomakan |
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.
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
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.
LGTM
Co-authored-by: 0xh3rman <[email protected]>
Hi @weixuefeng, could you please format the code using |
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.
LGTM
fix: #3934