monero-integrations / monerophp

Monero PHP library + JsonRPC Client
MIT License
119 stars 76 forks source link

Refactoring, code modernisation and some bug fixes #144

Closed ghost closed 1 year ago

ghost commented 1 year ago

I've done the following changes:

I haven't tested all the refactored code, so I might have inadvertently broken some things. Regardless, I strongly believe my intentions for this project are worthwhile and I'm open to collaborating with you on keeping this library up to date.

Thank you for your time!

BrianHenryIE commented 1 year ago

Hey, take a look at #133. There's some work being duplicated by a few of us!

I do particularly like:

On that last point, we should consider using a library like moneyphp/money or brick/money.

Good work.

refring commented 1 year ago

@BrianHenryIE I think using an 'Amount' class for working with piconero's and converting those to other denominations would suffice (did some work on that for my library), it would be a good fit for the separate monero php lib if things get split up in different packages.

ghost commented 1 year ago

@BrianHenryIE that's great! I should've checked for other requests beforehand. I'll wait for 143 to be merged then.

@refactor-ring's fork looks really good to me. Do you intend to eventually have it merged into the main repository?

refring commented 1 year ago

@BrianHenryIE that's great! I should've checked for other requests beforehand. I'll wait for 143 to be merged then.

@refactor-ring's fork looks really good to me. Do you intend to eventually have it merged into the main repository?

Thanks @Theophylactus , my project is not a fork though, and it doesn't include any code related to cryptography like generating keys but just implements the rpc methods which the current library does do.

recanman commented 1 year ago

I think the current RPC client implementation should be replaced with refactor-ring's implementation. Cryptography can be left for now. I've reviewed #143 — I think it should be a priority to get that merged now.

recanman commented 1 year ago

@Theophylactus I recommend looking at #141. It replaces SHA3.php with an MIT-licensed library that is faster. Unfortunately, I put it all in one commit, so you'd have to make the changes manually, but its pretty simple.

I've been swamped up with work, but I am making some time for getting the live tests to work, with generateblocks.

refring commented 1 year ago

@Theophylactus I recommend looking at #141. It replaces SHA3.php with an MIT-licensed library that is faster. Unfortunately, I put it all in one commit, so you'd have to make the changes manually, but its pretty simple.

I've been swamped up with work, but I am making some time for getting the live tests to work, with generateblocks.

@recanman What kind of tests are you working on exactly ? I've got some tests which work with generateblocks here and here , still writing more transfer tests.

recanman commented 1 year ago

I thought that you have not implemented those yet. I'll look at something else.