Conversation
damccull
force-pushed
the
fix_escrow_fee
branch
from
7976797 to
9225b07
Compare
|
@damccull it's still a draft? |
|
@ohager yeah I need someone who knows the escrow system to test it for correct behavior. |
hmm... then I'm the wrong person... hahaha - I think we need to reengineer |
|
I really think this is probably fine. I just removed the bits that check for the fee directly in the escrow API instead of letting the dynamic fee thing work. I just wanted someone more familiar with how to execute these transactions to verify them before we merge. |
| @Override | ||
| protected void validateAttachment(Transaction transaction) throws SignumException.ValidationException { | ||
| Attachment.AdvancedPaymentEscrowSign attachment = (Attachment.AdvancedPaymentEscrowSign) transaction.getAttachment(); | ||
| if (transaction.getAmountNqt() != 0 || transaction.getFeeNqt() != Constants.ONE_SIGNA) { |
There was a problem hiding this comment.
I think the minimum excrow transaction fee is supposed to be hard set to a minimum of 1 so this probably should be <= or just <
if (transaction.getAmountNqt() != 0 || transaction.getFeeNqt() <= Constants.ONE_SIGNA)There was a problem hiding this comment.
Hmm. The point of this PR is to /remove/ the hardcoded minimum so the dynamic minimums can be used like every other transaction. Don't remember who was mentioning it, but someone complained about it. Should we leave it at 1, like it already is? What's the justification for a higher 1-signum fee?
There was a problem hiding this comment.
I honestly don't know much about the escrow system. Asking why it's hard set to 1 is a mystery to me. We can go dynamic, but this would be a full team decision. @ohager @frankTheTank72
3 participants
@deleterium, can you build this version and test it out on the test network, see if it is working as you expect now and let me know if there's any issued?
@ohager there's a lot of formatting changes, but the key changes are in the final commit.
Resolves #677