Page MenuHomePhabricator

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

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

Details

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 Errors
SeverityLocationCodeMessage
Errormodules/ecash-agora/agora.py:20F401flake8 F401
Errormodules/ecash-agora/agora.py:20F401flake8 F401
Errormodules/ecash-agora/agora.py:20F401flake8 F401
Errormodules/ecash-agora/agora.py:20F401flake8 F401
Unit
No Test Coverage
Build Status
Buildable 30249
Build 60025: Build Diffecash-agora-integration-tests
Build 60024: arc lint + arc unit

Event Timeline

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`.Sep 11 2024, 13:51
modules/ecash-agora/tests/partial.slp.bigsats.test.ts
157 ↗(On Diff #49657)

isn't this the 2nd output?

161 ↗(On Diff #49657)

isn't this the third output?

244 ↗(On Diff #49657)

same question as above -- what is outputs[0] ?

modules/ecash-agora/tests/partial.test.slp.ts
1 ↗(On Diff #49657)

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.

modules/ecash-agora/tests/partial.slp.bigsats.test.ts
157 ↗(On Diff #49657)

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 ↗(On Diff #49657)

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 ↗(On Diff #49657)

Good point—maybe call it partial-helpers-slp.ts, or something else

Fabien requested changes to this revision.Sep 17 2024, 10:04
Fabien added a subscriber: Fabien.
Fabien added inline comments.
modules/ecash-agora/src/agora.ts
206 ↗(On Diff #49657)

no need for the else

266 ↗(On Diff #49657)

dito

399 ↗(On Diff #49657)

dito

596 ↗(On Diff #49657)

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.Sep 17 2024, 10:04

re this comment

image.png (136×604 px, 12 KB)

modules/ecash-agora/src/agora.ts
206 ↗(On Diff #49657)

I think the else has two good reasons:

  1. If a user specifies something other than ONESHOT and PARTIAL they should get an error (instead of falling into PARTIAL by default)
  2. If we add another variant, we can simply add another else if branch
399 ↗(On Diff #49657)

same reason here

modules/ecash-agora/agora.py
399 ↗(On Diff #49657)

image.png (500×958 px, 906 KB)

address review + based memes

This revision is now accepted and ready to land.Sep 24 2024, 19:55