-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow only positive transfer values #153
Conversation
@@ -256,6 +256,9 @@ export default class Wallet { | |||
transfer(input: {to: string, value: number, from?: string, nonce?: number, gas_price?: number}, isDryrun: boolean = false): Promise<any> { | |||
const address = this.getImpliedAddress(input.from); | |||
const toAddress = Ain.utils.toChecksumAddress(input.to); | |||
if (!(input.value > 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to check value is positive number? I know, If value is 0 or negative number, blockchain will throw error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this saves user's tx fee though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if the transaction fails, does the user still have to pay the transaction fee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good quesiton.
It depends on the conditions the transaction breaks.
If the're prechecks or skipFees = true, tx fee is not charged (see https://github.com/ainblockchain/ain-blockchain/blob/master/db/index.js#L1955).
Basically, for the failed transactions included in blocks, tx fee is charged.
It's for handling dos attacks using failed transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some question. Thanks!
Thanks for the review! @woomurf Now merging.. |
Change summary:
Related issues: