-
Notifications
You must be signed in to change notification settings - Fork 88
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
Fix getAccountsWithOptionalFeePayer #184
Fix getAccountsWithOptionalFeePayer #184
Conversation
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
==========================================
+ Coverage 80.37% 80.39% +0.02%
==========================================
Files 143 143
Lines 3108 3112 +4
==========================================
+ Hits 2498 2502 +4
Misses 610 610
Continue to review full report at Codecov.
|
Hey @ChikinDeveloper Can you please add some tests? Also, this order should already be taken care of by the fact that |
Hi @ookami-kb, added a test |
I see. Are there some docs on why the program IDs should be placed at the end of the account list? The only information I can find is this one:
And this order is respected by |
No, there isn't any doc about it, in fact the transaction will work even if you don't place the program ids at the end. |
Yeah, I see. Well, that can be a problem – relying on the implementation details of a specific library. There's no guarantee that this implementation won't change in the future. By the way, as I see it from the source code, they rather sort signatures in the lexicographical order, not just "program ids go last". Can you please provide more details on your use case? I'm just trying to understand where's the connection between dart and JS libraries. |
Just checked the rust version, it's the same they place program ids at the end source. Indeed, I forgot to check this part, good job spotting that! We have a flutter web app, and try to interact with Phantom chrome extension. So we need to convert from dart to js and back in order to call Phantom with a javascript Transaction object. I can imagine an other (very specific) use case : I understand that there's no guarantee that this implementation won't change in the future. And it isn't even needed since the transactions will work as long as you sort signed/writeable accounts (and feePayer). Moreover if the implementation change in the future, this may lead to issues with all the websites that uses the library, if their version differs from the version used to a wallet they connect to. So I guess it should be written somewhere in the solana doc (in a developer standard section or something like that) |
The drawback that I see there is that the dart library would depend on the undocumented implementation of the js library. Which in the future will lead to the situation that a specific js library version is only compatible with a specific dart library version. Also, while the Rust library indeed places program IDs at the end, it doesn't do this lexicographic sorting, as far as I see it. So Rust and JS implementations are different as well. So the same situation can in theory happen with mixing dart <-> rust code.
In that case, I would rather say that since API accepts a signature and uses format-specific serialization, it should return a compiled message instead of relying on client knowledge about implementation details of the back-end. Otherwise, it just looks like bad architecture. In your case, I would say it's better to add some layer that would actually transform the instructions by reordering accounts to match the order from JS library. I understand that it will introduce some overhead, but I don't think that one library should rely on some "random" implementation details from another library, especially when they are:
|
Here is the commit that added locale lexicographical sorting I say "Fix non deterministic writeable account order" so I guess it has been added to prevent this kind of issue. (But maybe sorting by locale isn't the best idea ever haha)
|
I found this : solana-labs/solana#21722 |
Fix getAccountsWithOptionalFeePayer.
Had issues signing transactions with phantom wallet, took 3 days to find out why haha
Source code for transaction encoding
The programIds needs to be placed at the end the accounts list