-
Notifications
You must be signed in to change notification settings - Fork 76
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
draft: Final touches (closes #147, #135, #133, #132, #124, #111, #100, #92, #82, #81, #57, #54) #154
base: master
Are you sure you want to change the base?
Conversation
credits: refring
An extremely important task is to have tests for these cryptographic operations (to make sure results are consistent of course). If anyone seeing this is willing to give an expected input/output for each function in Cryptonote.php while I'm refactoring, that would be really great (if it isn't done, I'll do it myself. Just in case anyone else wants to speed up the migration process). |
Rewriting it with sodium does not seem feasible at the moment as certain functions are not too well-documented. I've decided to add types, tests, and clean up the code for Here's my progress on
|
b6a1c60
to
69b9a6f
Compare
Just added the publishing workflow. I'm about 80% done with the rest of the refactoring, but yet to polish it up and push it here. |
@recanman I'd like to help completing any remaining tasks, performing tests, adding unit tests, etc. ... I'll be reviewing this as it progresses and the other repo as well, if there's any areas still needing work please feel free to direct me towards it. |
I really appreciate your help. I've been a bit busy. |
I think that helping to implement the remaining functions would be great. I have struggled with |
Thanks to both of you. |
@recanman I'm working on this. I'm syncing testnet and getting set up to run ... basing off of your
and so on due to the classes not being found / imported properly. I'm just running Later I can update the documentation. For now I'm going to poke around and any tips would be appreciated Also, can you recommend an IDE? I usually use JetBrains IDEs but meh |
Just documenting as I go for reference later... I installed Composer, ran
but I still get
which I'll work on massaging... Are you using a globally-installed PHPUnit or are you installing PHPUnit with Composer? And I see you're on Windows by your file format, I'll have to set my git to use Windows-style line endings... I also have a Windows dev box which I can work on for convenience sake. We should document the setup required for both Windows and Linux anyways. I also have a fresh macOS :D |
Let me check my local machine, sorry about that. |
After running By the way, try |
Ahh I was just running I have to take a break for awhile, if there are no more pushes by that time I'll start tinkering on what's here, thanks for this, love the approach to reimplement crypto fundamentals in PHP. I'll also contribute over in the rpc-centric repo. Cheers |
I have pushed all of my local changes. Most of the remaining Cryptonote functions are dependent on (CI now passes! 😄) |
https://getmonero.dev/cryptography/asymmetric/private-key.html I am rewriting the README to get it back up-to-date, and also to reduce confusion on how to run tests. |
349860f
to
d3d8223
Compare
OK, just confirming that I got the tests working basically:
so I've begun working and will be back with updates as they come. note that this doesn't imply any review and these sorts of changes will require thorough review |
Well, we have been stuck on the crypto part, as expected. |
Sorry I haven't been of help there @recanman, been doing lots in lots of different languages lately. I recently made a proof of concept binding MrCyjaneK's monero_c (monero-project/monero C wrapper) to Rust here. An FFI approach by me would use this same approach: a monero.php project in monero_c/impls. I searched and saw a few good articles about FFI and PHP that make the effort seem feasible, I'll make a proof of concept as soon as I find some spare time |
@recanman I was able to pull up a simple PHP implementation: <?php
$content = file_get_contents("../../monero_libwallet2_api_c/src/main/cpp/wallet2_api_c.h");
$lines = explode("\n", $content);
$exclude = array();
$i = 0;
foreach ($lines as $line) {
if (!str_starts_with($line, " ")) {
if (strpos($line, 'MONERO_') == FALSE) {
continue;
}
}
$i++;
$line = str_replace("extern ADDAPI ", "", $line);
$exclude[] = $line;
// echo "$i: $line\n";
}
$content = implode("\n", $exclude);
// echo $content;
$ffi = FFI::cdef($content, "../../release/monero/x86_64-linux-gnu_libwallet2_api_c.so");
$wmPtr = $ffi->MONERO_WalletManagerFactory_getWalletManager();
$exists = $ffi->MONERO_WalletManager_walletExists($wmPtr, "test_wallet");
if ($exists) {
$wPtr = $ffi->MONERO_WalletManager_openWallet($wmPtr, "test_wallet", "test_password", 0);
} else {
$wPtr = $ffi->MONERO_WalletManager_createWallet($wmPtr, "test_wallet", "test_password", "English", 0);
}
$status = $ffi->MONERO_Wallet_status($wPtr);
if ($status != 0) {
$error = $ffi->MONERO_Wallet_errorString($wPtr);
echo "Unable to create wallet: $error".PHP_EOL;
exit(1);
}
echo $ffi->MONERO_Wallet_address($wPtr, 0, 0).PHP_EOL; which works in the cli
If that looks like something that could help you I'm happy to assist you with adopting monero_c for PHP :) |
Thanks for the proof of concept @MrCyjaneK! That's a demo of outsourcing crypto to the cpp, @recanman :) |
Yes, I see. Thanks for the clarification. For some reason, I assumed it was connecting to RPC. Sorry about that. |
@recanman I want to focus on Rust for a bit, so will do PHP sometime after that (and other work tasks). If you want it done quicker than that--and done in a way that that FCMPs etc. will be easier for us to adopt in PHP instead of reimplementing ("rolling our own") crypto in pure PHP--MrCyjaneK/monero_c#70 is there ready to complete or serve as an example this also serves as a great opportunity for us to compare these new PHP impl outputs to the monero-project/monero impls directly, right in PHP |
@serhack I support the updating of monerophp from its current state to this migration proposed by @recanman. This will get it updated and working with Monero's latest release. Although it is not feature-complete, it is very close and this PR would make the repo overall much more useful. @recanman ready to mark this PR as ready for review? |
I suggest that this also gets merged: monero-integrations/monerophp-rpc#1 |
Ping me whenever it's ready :) I'll take another close look and then I'll merge it |
This pull request contains the final changes needed for the package migration.
Package name will be changed to
monero-integrations/monero-crypto
(namespaceMoneroIntegrations\MoneroCrypto
)This will also bump the minimum PHP version to 8.1.
A fork of refring's work. This will be called
monero-integrations/monero-rpc
(namespaceMoneroIntegrations\MoneroRpc
).Tasks left:
MoneroRpc
packageMost of my work is usually offline/on my local git server and I may not push to this PR's branch frequently.
I've spent some time with refring's code and tested it (it is very good). Merging it is just a matter of creating a new repository (I will CC serhack at the appropriate time)