-
Notifications
You must be signed in to change notification settings - Fork 246
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
Add a partial support for vote program #167
Conversation
@unordered-set can you write a test? |
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
- Coverage 91.06% 90.97% -0.10%
==========================================
Files 37 39 +2
Lines 2698 2725 +27
==========================================
+ Hits 2457 2479 +22
- Misses 241 246 +5 |
It looks I've found an issue, but not sure where exactly.
The message above is decoded correctly, but it has a fee-payer different from the withdrawer. So I've updated it to:
Nice, only one withdrawer. But now the message fails to deserialize.
so the way how num accounts is computed is wrong somewhere (either on solana-cli or on solana-py). Checking.
|
What are you using to get this |
These are debug prints I placed into the library (which is located in this repo) |
Ok, these are binary dumps of txs:
|
Not sure what to do with those 😅 I'm trying to find the line that's hanging |
The hanging line is here: https://github.com/michaelhly/solana-py/blob/master/src/solana/message.py#L180 as num accounts is huge. So I'm trying to figure out if it is an issue with solana-cli serialization or this library deserialization |
So, in case of two accounts reading with offset 64*2+1 |
These look like transaction messages not transactions. Messages don't have the signatures array prepended |
Yeah if I call
|
Yes, I see, it was a coincidence, that it was also correctly interpreted as transaction in the first byte. Guys , thanks a lot and sorry for bothering you |
No worries, thank you for speaking out when there appeared to be a problem! Guess we can get back to this PR now |
Rust transaction sanitization is done here. Might pay to have similar logic and raise more helpful errors |
@kevinheavey @michaelhly , Added tests, do you mind making one more review iteration please? |
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!
Thank you @michaelhly ! I just don't have write permissions to click merge :) |
Transaction generated by this code: https://explorer.solana.com/tx/5GepwTxXrGpPPtLbY1VtKomHLqTRd3FozwK14G22yUPQuL9MBa5CeCkxuxY1Mcb9jHgvFF3ncCRZVbCu2ZVbquTu?cluster=testnet