Page MenuHomePhabricator

[ecash-agora] Add validation to _acceptTxBuilder to prevent creation of unspendable offers
ClosedPublic

Authored by bytesofman on Sun, Oct 20, 00:21.

Details

Summary

It's possible for an agora partial offer to be accepted in such a way that the remaining quantity is no longer possible to accept. This could happen in 2 ways.

  1. The remaining quantity is below the contract min accepted qty.
  2. The remaining quantity costs less than dust. (Probably still possible to accept this? Though only by overpaying).

Add validation to prevent this type of accept. App devs should also include this type of validation in the front end.

The validation can be disabled by overriding a default param. This is helpful for integration tests, and also allows this lib to cover the full features of the protocol.

Test Plan

npm test, CI

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Failed tests logs:

====== AgoraPartial ALP: AgoraPartial ALP 1000 for 1sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 1000 for 1sat/token, dust accept ======
Error: Accepting 546 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.
    at AgoraOffer._acceptTxBuilder (src/agora.ts:270:35)
    at AgoraOffer.acceptTx (src/agora.ts:149:30)
    at takeAlpOffer (tests/partial-helper-alp.ts:140:35)
    at Context.<anonymous> (tests/partial.alp.test.ts:463:38)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
====== AgoraPartial SLP: AgoraPartial SLP 1000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000 for 1sat/token, dust accept ======
Error: Accepting 546 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.
    at AgoraOffer._acceptTxBuilder (src/agora.ts:270:35)
    at AgoraOffer.acceptTx (src/agora.ts:149:30)
    at takeSlpOffer (tests/partial-helper-slp.ts:144:35)
    at Context.<anonymous> (tests/partial.slp.test.ts:518:38)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Each failure log is accessible here:
AgoraPartial ALP: AgoraPartial ALP 1000 for 1sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 1000 for 1sat/token, dust accept
AgoraPartial SLP: AgoraPartial SLP 1000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000 for 1sat/token, dust accept

patch tests to accept creating unspendable partial outputs for failing cases

Failed tests logs:

====== AgoraPartial SLP: AgoraPartial SLP 1000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000 for 1sat/token, dust accept ======
Error: Accepting 546 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.
    at AgoraOffer._acceptTxBuilder (src/agora.ts:270:35)
    at AgoraOffer.acceptTx (src/agora.ts:149:30)
    at takeSlpOffer (tests/partial-helper-slp.ts:145:35)
    at Context.<anonymous> (tests/partial.slp.test.ts:520:38)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Each failure log is accessible here:
AgoraPartial SLP: AgoraPartial SLP 1000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000 for 1sat/token, dust accept

add test for error catching

Failed tests logs:

====== AgoraPartial ALP: AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept ======
AssertionError: expected promise to be rejected with an error including 'Accepting 503316480 token satoshis wo…' but it was fulfilled with '88a7c956950eebefaea7ba6e230264c9c7229…'

      + expected - actual

      -88a7c956950eebefaea7ba6e230264c9c7229633d4ed73d35525d1e3d1008b84
      +Accepting 503316480 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.
====== AgoraPartial SLP: AgoraPartial SLP 1000000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000000 for 1sat/token, dust accept ======
AssertionError: expected promise to be rejected with an error including 'Accepting 546 token satoshis would le…' but it was fulfilled with '00e016ec19e4c52e92e1e237a12c682f97327…'

      + expected - actual

      -00e016ec19e4c52e92e1e237a12c682f97327f3b9fda8b6e8590d849d64ed36d
      +Accepting 546 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.

Each failure log is accessible here:
AgoraPartial ALP: AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept
AgoraPartial SLP: AgoraPartial SLP 1000000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000000 for 1sat/token, dust accept

Failed tests logs:

====== AgoraPartial ALP: AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept ======
AssertionError: expected promise to be rejected with an error including 'Accepting 503316480 token satoshis wo…' but it was fulfilled with 'e5c7424a538864b0d5147850215c2c432838e…'

      + expected - actual

      -e5c7424a538864b0d5147850215c2c432838ea58b82b911d890786331fcd9a1f
      +Accepting 503316480 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.
====== AgoraPartial SLP: AgoraPartial SLP 1000000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000000 for 1sat/token, dust accept ======
AssertionError: expected promise to be rejected with an error including 'Accepting 546 token satoshis would le…' but it was fulfilled with '83c2ec78b0d0714f04251251d5f42451faf5c…'

      + expected - actual

      -83c2ec78b0d0714f04251251d5f42451faf5c97f9001ee636b00895c8547c291
      +Accepting 546 token satoshis would leave an amount lower than the min acceptable by the terms of this contract, and hence unacceptable. Accept fewer tokens or the full offer.

Each failure log is accessible here:
AgoraPartial ALP: AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept.AgoraPartial ALP AgoraPartial ALP 281474976710655 for 0.000001sat/token, dust accept
AgoraPartial SLP: AgoraPartial SLP 1000000 for 1sat/token, dust accept.AgoraPartial SLP AgoraPartial SLP 1000000 for 1sat/token, dust accept

emack requested changes to this revision.Tue, Oct 22, 12:58
emack added a subscriber: emack.
emack added inline comments.
modules/ecash-agora/src/agora.ts
257–258 ↗(On Diff #50255)

are these the same? i.e. if this param is set to false then it's already not an undefined value?

modules/ecash-agora/tests/partial.alp.test.ts
621 ↗(On Diff #50255)

but you are manually setting an override here though?

This revision now requires changes to proceed.Tue, Oct 22, 12:58
bytesofman added inline comments.
modules/ecash-agora/src/agora.ts
257–258 ↗(On Diff #50255)

I don't think JS will be happy if we check for params.allowUnspendable === false and it is actually undefined. I think it will throw an error on the check.

... that said, mb not, I'll give it a try.

ok yeah I can just remove it and behavior is the same. Removed.

modules/ecash-agora/tests/partial.alp.test.ts
621 ↗(On Diff #50255)

right. There's a failing legacy test case on ALP and SLP now that we are, by default, not allowing the creation of unspendable remainders.

To show that this can still pass in the same way as before if allowUnspendable is specified -- it's specified.

bytesofman marked an inline comment as done.

rebase, remove unnecessary typeof !== undefined check

This revision is now accepted and ready to land.Tue, Oct 22, 23:42
tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
modules/ecash-agora/src/agora.ts
240 ↗(On Diff #50281)

this should be factored into its own public function so people can verify the amount before putting it into the accept... function

This revision now requires changes to proceed.Wed, Oct 30, 16:29

rebase, split out validation into public method for partials, preventUnacceptableRemainder

back out unrelated lint change

Otherwise lgtm

modules/ecash-agora/src/agora.ts
241 ↗(On Diff #50566)

If undefined is provided, it should check it also

modules/ecash-agora/src/partial.ts
573 ↗(On Diff #50566)
This revision is now accepted and ready to land.Fri, Nov 1, 19:19
bytesofman marked an inline comment as done.

rebase, fix antipattern in validation (return if we have no remainder instead of only check if we do), make sure to validate if allowUnspendable is undefined, confirm with test