[ecash-agora] Partial Agora offers: Add SLP support to plugin, `Agora` and `AgoraOffer`
Needs RevisionPublic
Actions

Authored by tobias_ruck on Wed, Sep 11, 13:31.

Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Summary

In D16743 script was added, and now we make it usable by both indexing these offers via the agora.py plugin, as well as adding support to accepting/cancelling them via the Agora class, and AgoraOffer, respectively.

Depends on D16743.

Test Plan

npm run integration-tests

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ecash-agora-partial-offer-slp

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30306
Build 60139: Build Diff	ecash-agora-integration-tests · ecash-agora-tests
Build 60138: arc lint + arc unit

Event Timeline

tobias_ruck created this revision.Wed, Sep 11, 13:31

Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Sep 11, 13:31

Harbormaster completed remote builds in B30249: Diff 49584.Wed, Sep 11, 13:31

fix lints

tobias_ruck retitled this revision from [ecash-agora] Partial Agora offers: Add support to plugin, `Agora` and `AgoraOffer` to [ecash-agora] Partial Agora offers: Add SLP support to plugin, `Agora` and `AgoraOffer`.Wed, Sep 11, 13:51

Harbormaster completed remote builds in B30250: Diff 49586.Wed, Sep 11, 14:50

tobias_ruck published this revision for review.Wed, Sep 11, 15:31

tobias_ruck added a child revision: D16753: [ecash-agora] Add ALP support to `AgoraPartial.script`.Thu, Sep 12, 22:40

tobias_ruck mentioned this in D16755: [ecash-agora] Partial Agora offers: Add ALP support to plugin, `Agora` and `AgoraOffer`.Thu, Sep 12, 23:27

rebase onto D16743

Harbormaster completed remote builds in B30306: Diff 49657.Mon, Sep 16, 19:35

bytesofman added inline comments.Mon, Sep 16, 21:05

modules/ecash-agora/tests/partial.slp.bigsats.test.ts
157	isn't this the 2nd output?
161	isn't this the third output?
244	same question as above -- what is outputs[0] ?
modules/ecash-agora/tests/partial.test.slp.ts
1	so we have `partial.slp.test.ts` and also `partial.test.slp.ts` ? I can't really tell from the naming what I should expect to find in each file / why they are distinct.

tobias_ruck added inline comments.Tue, Sep 17, 07:30

modules/ecash-agora/tests/partial.slp.bigsats.test.ts
157	yeah well the OP_RETURN is the 0th output 🙃 You would've hoped there's a convention for this already, but I can change it for clarity
244	outputs[0] is the OP_RETURN I can add a test that checks if its Script starts with OP_RETURN "SLP\0" if that helps :)
modules/ecash-agora/tests/partial.test.slp.ts
1	Good point—maybe call it `partial-helpers-slp.ts`, or something else

Fabien requested changes to this revision.Tue, Sep 17, 10:04

Fabien added a subscriber: Fabien.

Fabien added inline comments.

modules/ecash-agora/src/agora.ts
206	no need for the else
266	dito
399	dito
596	Reviewer note: [0] is fine because the number of bytes is limited by design, and if > 255 it's going to be invalid

This revision now requires changes to proceed.Tue, Sep 17, 10:04

re this comment

tobias_ruck added inline comments.Thu, Sep 19, 09:43

modules/ecash-agora/src/agora.ts
206	I think the `else` has two good reasons: If a user specifies something other than ONESHOT and PARTIAL they should get an error (instead of falling into PARTIAL by default) If we add another variant, we can simply add another else if branch
399	same reason here