Skip to content

don't allow multiple outputs on TXN fees#142

Merged
cam-parra merged 3 commits into
sovrin-foundation:masterfrom
devin-fisher:fix_TOK_387-2
Sep 5, 2018
Merged

don't allow multiple outputs on TXN fees#142
cam-parra merged 3 commits into
sovrin-foundation:masterfrom
devin-fisher:fix_TOK_387-2

Conversation

@devin-fisher
Copy link
Copy Markdown

fix for TOK-387

Signed-off-by: Devin Fisher devin.fisher@evernym.com

Devin Fisher added 3 commits September 4, 2018 16:11
fix for TOK-387

Signed-off-by: Devin Fisher <devin.fisher@evernym.com>
Signed-off-by: Devin Fisher <devin.fisher@evernym.com>
Copy link
Copy Markdown

@cam-parra cam-parra left a comment

Choose a reason for hiding this comment

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

This looks good to me but a quick non-blocking question: Has the todo in https://github.com/evernym/plugin/pull/142/files#diff-8b0c45456d245ad53d1b31121aaf4a48R111 been resolved?

@devin-fisher
Copy link
Copy Markdown
Author

I don't know. I would have to look into it.

@cam-parra cam-parra merged commit 530167a into sovrin-foundation:master Sep 5, 2018
if len(outputs) > MAX_FEE_OUTPUTS:
raise InvalidClientRequest(request.identifier,
request.reqId,
"Only {} OUTPUT is allow for Transaction fees".format(MAX_FEE_OUTPUTS))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

allowed instead of allow

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That would be a good change

change_address = [change_address]
change_part = change // len(change_address)
if change_part <= 0:
raise Exception("Unable to divide {}, {} ways".format(change, len(change_address)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't see how this exception will be reached. Len will never be less than 0, and this is under an if block that enforces the change to be greater than 0. So the change_part will never be negative and won't run.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I can only happen if the fee is lower then the number of output addresses.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Okay. I see it now. That's weird. Thanks.

req,
utxos,
fee_amount,
change_address=[address_main, Address().address]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use helpers.wallet.create_address rather than the Address class directly. Otherwise the signing should fail.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Did not know about that. That would be a good improvement.

@devin-fisher devin-fisher deleted the fix_TOK_387-2 branch September 7, 2018 17:39
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