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.

The remaining quantity is below the contract min accepted qty.
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 30694
Build 60904: Build Diff	ecash-agora-tests · ecash-agora-integration-tests
Build 60903: arc lint + arc unit

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

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

patch tests to accept creating unspendable partial outputs for failing cases

Harbormaster completed remote builds in B30697: Diff 50233.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

update missed test

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

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

check errors first

Harbormaster completed remote builds in B30712: Diff 50251.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.

clean up tests

Harbormaster completed remote builds in B30714: Diff 50254.Oct 21 2024, 22:24

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.

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

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

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.

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

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….