Page MenuHomePhabricator
Differential D16944

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

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

Details

Reviewers
emack
Group Reviewers
Restricted Project
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
Branch
agora-validate-accepts
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30699
Build 60914: Build Diffecash-agora-integration-tests · ecash-agora-tests
Build 60913: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sun, Oct 20, 00:21
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Oct 20, 00:21
Harbormaster completed remote builds in B30694: Diff 50228.Sun, Oct 20, 00:27
teamcity added a comment.Sun, Oct 20, 00:27
Build Bitcoin ABC Diffs / Diff Testing (ecash-agora-integration-tests) failed.

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

bytesofman updated this revision to Diff 50233.Sun, Oct 20, 10:57

patch tests to accept creating unspendable partial outputs for failing cases

Harbormaster completed remote builds in B30697: Diff 50233.Sun, Oct 20, 11:03
teamcity added a comment.Sun, Oct 20, 11:03
Build Bitcoin ABC Diffs / Diff Testing (ecash-agora-integration-tests) failed.

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

bytesofman updated this revision to Diff 50235.Sun, Oct 20, 21:00

update missed test

bytesofman updated this revision to Diff 50236.Sun, Oct 20, 21:06

add test for error catching

Harbormaster completed remote builds in B30698: Diff 50235.Sun, Oct 20, 21:10
Harbormaster completed remote builds in B30699: Diff 50236.Sun, Oct 20, 21:18
teamcity added a comment.Sun, Oct 20, 21:18
Build Bitcoin ABC Diffs / Diff Testing (ecash-agora-integration-tests) failed.

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

bytesofman updated this revision to Diff 50251.Mon, Oct 21, 18:26

check errors first

Harbormaster completed remote builds in B30712: Diff 50251.Mon, Oct 21, 18:35
teamcity added a comment.Mon, Oct 21, 18:35
Build Bitcoin ABC Diffs / Diff Testing (ecash-agora-integration-tests) failed.

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

bytesofman updated this revision to Diff 50254.Mon, Oct 21, 22:18

clean up tests

Harbormaster completed remote builds in B30714: Diff 50254.Mon, Oct 21, 22:24
bytesofman updated this revision to Diff 50255.Mon, Oct 21, 22:43

remove if block

Harbormaster completed remote builds in B30715: Diff 50255.Mon, Oct 21, 22:48
bytesofman published this revision for review.Mon, Oct 21, 22:54
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 marked an inline comment as done.Tue, Oct 22, 16:25
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 updated this revision to Diff 50281.Tue, Oct 22, 16:26
bytesofman marked an inline comment as done.

rebase, remove unnecessary typeof !== undefined check

Harbormaster completed remote builds in B30735: Diff 50281.Tue, Oct 22, 16:42
emack accepted this revision.Tue, Oct 22, 23:42
This revision is now accepted and ready to land.Tue, Oct 22, 23:42
bytesofman mentioned this in D16973: [ecash-agora] Catch up changelog for next published version.Wed, Oct 23, 23:51
bytesofman mentioned this in rABC77bc58465ccc: [ecash-agora] Catch up changelog for next published version.Thu, Oct 24, 03:20

Revision Contents
Changeset List

PathSize
modules/
ecash-agora/
src/
49 lines
tests/
2 lines
2 lines
22 lines
22 lines

Diff 50236

View Options

modules/ecash-agora/src/agora.ts

Loading...
View Options

modules/ecash-agora/tests/partial-helper-alp.ts

Loading...
View Options

modules/ecash-agora/tests/partial-helper-slp.ts

Loading...
View Options

modules/ecash-agora/tests/partial.alp.test.ts

Loading...
View Options

modules/ecash-agora/tests/partial.slp.test.ts

Loading...