Skip to content
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 payment processor #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

LEAD-Anoy74
Copy link

Fix payment processor for all coins.

Fix payment processor for all coins.
@mooleshacat
Copy link

Looks good 👍

@@ -577,7 +580,7 @@ logger.info(logSystem, logComponent, addressAmounts);


var getProperAddress = function(address){
if (address.length === 40){
if (address.length === 60){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whys this changed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That portion of code doesn't even make sense to exist anymore, since miners should be allowed to setup a longer .workername.

It's also broken, and doesn't even change the address to poolconfig pooladdrress.

Cheers

Copy link

@mooleshacat mooleshacat Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree partially with @LEAD-Anoy74 on the issue of the length should not even be checked.

The function is not meant to change the address at all... It is meant to check the address if it is valid and matches pool configuration.

Most pools accept WALLET.WORKER format and the workername can be any length (unless pool limits it)

What I would do is split the address / worker name and then do the length check after, or just rip it out alltogether.

Either use:

var getProperAddress = function(address){

    var splitted_address = [address].toString();
    splitted_address = splitted_address.split('.',1);

    if (splitted_address.length === 60){
            return util.addressFromEx(poolOptions.address, splitted_address);
    fi
    else return splitted_address;

}

or use this:

var getProperAddress = function(address){

    return util.addressFromEx(poolOptions.address, address);

}

I would personally modify the function and add more sanity / validation checks.

For example:

  • Add in a check to make sure there are no symbols inside the string containing the address/worker name (except for the '.' separating the two)
  • Add a validateaddress RPC call to check the address is valid
  • Use the same validateaddress RPC call results to check the address is owned by the pool daemon

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spacing of this is all over the place, but from a quick skim, the function can be kept the same by splitting the address before its passed into getProperAddress. The purpose of this function is as a sanity check to ensure that only a valid base58 bitcoin-type address is passed in and used from that point forward. This change would be breaking that check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants