Page MenuHomePhabricator
Differential D16944

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

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

Details

Reviewers
emack
tobias_ruck
Group Reviewers
Restricted Project
Commits
rABCb27c743f6c10: [ecash-agora] Add validation to _acceptTxBuilder to prevent creation of…
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

bytesofman created this revision.Oct 20 2024, 00:21
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 20 2024, 00:21
Harbormaster completed remote builds in B30694: Diff 50228.Oct 20 2024, 00:27
teamcity added a comment.Oct 20 2024, 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.Oct 20 2024, 10:57

patch tests to accept creating unspendable partial outputs for failing cases

Harbormaster completed remote builds in B30697: Diff 50233.Oct 20 2024, 11:03
teamcity added a comment.Oct 20 2024, 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.Oct 20 2024, 21:00

update missed test

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

add test for error catching

Harbormaster completed remote builds in B30698: Diff 50235.Oct 20 2024, 21:10
Harbormaster completed remote builds in B30699: Diff 50236.Oct 20 2024, 21:18
teamcity added a comment.Oct 20 2024, 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.Oct 21 2024, 18:26

check errors first

Harbormaster completed remote builds in B30712: Diff 50251.Oct 21 2024, 18:35
teamcity added a comment.Oct 21 2024, 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.Oct 21 2024, 22:18

clean up tests

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

remove if block

Harbormaster completed remote builds in B30715: Diff 50255.Oct 21 2024, 22:48
bytesofman published this revision for review.Oct 21 2024, 22:54
emack requested changes to this revision.Oct 22 2024, 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.Oct 22 2024, 12:58
bytesofman marked an inline comment as done.Oct 22 2024, 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.Oct 22 2024, 16:26
bytesofman marked an inline comment as done.

rebase, remove unnecessary typeof !== undefined check

Harbormaster completed remote builds in B30735: Diff 50281.Oct 22 2024, 16:42
emack accepted this revision.Oct 22 2024, 23:42
This revision is now accepted and ready to land.Oct 22 2024, 23:42
bytesofman mentioned this in D16973: [ecash-agora] Catch up changelog for next published version.Oct 23 2024, 23:51
bytesofman mentioned this in rABC77bc58465ccc: [ecash-agora] Catch up changelog for next published version.Oct 24 2024, 03:20
tobias_ruck requested changes to this revision.Wed, Oct 30, 16:29
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
bytesofman updated this revision to Diff 50565.Wed, Oct 30, 19:38

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

bytesofman updated this revision to Diff 50566.Wed, Oct 30, 19:40

back out unrelated lint change

Harbormaster completed remote builds in B30932: Diff 50566.Wed, Oct 30, 19:48
Harbormaster completed remote builds in B30931: Diff 50565.
tobias_ruck accepted this revision.Fri, Nov 1, 19:19

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 updated this revision to Diff 50614.Fri, Nov 1, 22:56
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

Harbormaster completed remote builds in B30968: Diff 50614.Fri, Nov 1, 23:06
Closed by commit rABCb27c743f6c10: [ecash-agora] Add validation to _acceptTxBuilder to prevent creation of… (authored by bytesofman). · Explain WhyFri, Nov 1, 23:08
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCb27c743f6c10: [ecash-agora] Add validation to _acceptTxBuilder to prevent creation of….

Revision Contents
Changeset List

PathSize
modules/
ecash-agora/
src/
16 lines
42 lines
tests/
2 lines
2 lines
118 lines
149 lines

Diff 50615

View Options

modules/ecash-agora/src/agora.ts

Loading...
View Options

modules/ecash-agora/src/partial.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...