Page MenuHomePhabricator

[ecash-agora] Partial Agora offers: Add approximation logic
ClosedPublic

Authored by tobias_ruck on Sep 7 2024, 16:16.

Details

Summary

Add AgoraPartial and AgoraPartialParams for offers of fungible tokens, and add (only) the script param approximation logic.

This is rather complex, and is explained in the comments. The corresponding Script can be seen in D16644, which will be added in later diffs.

Test Plan

npm run integration-tests

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-agora-partial-approx
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30239
Build 60005: Build Diffecash-agora-integration-tests
Build 60004: arc lint + arc unit

Event Timeline

move approx test into src

add many more test cases, improve approximation in some cases

Fabien added inline comments.
modules/ecash-agora/src/partial.ts
222 ↗(On Diff #49544)

Nit: this is probably quite obvious but scriptUnsignedIntegerBits is more accurate (or just scriptUintBits)

235 ↗(On Diff #49544)

Should it be 0x7fffffffffffffffn to avoid negative values ?

243 ↗(On Diff #49544)

same question

251 ↗(On Diff #49544)

Dito, maxScriptUint

253 ↗(On Diff #49544)
274 ↗(On Diff #49544)

This comment confused me, I thought you were referring to the max supply somehow with this 21M value :) Please use 2^31 to clarify

modules/ecash-agora/src/partial.ts
222 ↗(On Diff #49544)

hmm, I get your point, but I don't really like either of those names, the "unsigned" seems misleading.

we could also keep scriptIntegerBits, but then have it be 32 (and 64) respectively, and simply subtract the sign bit

or maybe better numScriptIntegerBits

235 ↗(On Diff #49544)

No, this is what the SLP spec allows, so we should follow it.

We could go higher (and truncate more), but that ofc woudn't make sense since these numbers couldn't be represented in SLP

243 ↗(On Diff #49544)

same here, this is simply the max number for ALP values

251 ↗(On Diff #49544)

here it's literally the max script integer, so I think this is fine

274 ↗(On Diff #49544)

good point :)

modules/ecash-agora/src/partial.ts
222 ↗(On Diff #49544)

I like the idea of setting to 32 and apply -1 on the sign bit

404–406 ↗(On Diff #49544)

This works and I might just be too used to bit masks, but I prefer this version. Your call.

428–429 ↗(On Diff #49544)

Same, feel free to ignore if you disagree

modules/ecash-agora/src/partial.ts
37 ↗(On Diff #49544)
118 ↗(On Diff #49544)

what is the most inaccurate prices can be in this current implementation?

modules/ecash-agora/src/partial.ts
118 ↗(On Diff #49544)

There's some degenerate cases, especially when accepting just 1 token of something

The tests go through different scenarios, some are exactly accurate, some are quite a bit off. For example, "AgoraPartial.approximateParams 2p63-1, small price" is off by 190x (15258 nsats/token instead of 80) for accepting 0x10000000000 tokens, but that's an extreme example.

You can see the others are mostly within 5% for the smallest accept amount, and if you even accept like 5 or 10 accuracy goes up quickly.

We can refine the estimation later over time, without changing the script, so only makers would need to upgrade to get the better precision.

404–406 ↗(On Diff #49544)

I tried this version but it turned out more complicated IMO.

Also while this may be very normal bit arithmetic for C++ devs, it's more novel for TS devs, so I try to use simpler operations (like shifting right then left is IMO easier to understand than shifting and then subtracting 1)

428–429 ↗(On Diff #49544)

Yes, again I'm just not convinced this is more obvious to a TS dev this way

improve comments, use 32 for bits, address other nits

add test to build, fix test

This revision is now accepted and ready to land.Sep 11 2024, 08:50