Page MenuHomePhabricator
Differential D17063

[Cashtab] Patch bug in manage agora offers
ClosedPublic
Actions

Authored by bytesofman on Sat, Nov 2, 14:36.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC5b9ca8ccad18: [Cashtab] Patch bug in manage agora offers
Summary

When Orderbook was refactored so that each component owned its offer lookups, did not properly manage the distinction in how we fetch offers by pub key (agora call returns all offers, including multiple offers for the same token) and other offers (agora call returns tokenIds only, uniquely).

Correct this by using a Set to track tokenIds of managed offers. Convert to a sorted array before rendering.

Test Plan

npm test

Create two offers with the same token ID
npm start
navigate to Agora page
toggle "manage" switch
no "duplicate key" error, no repeated orderbooks

Diff Detail

Repository
rABC Bitcoin ABC
Branch
agora-patch-ownoffers
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30971
Build 61445: Build Diffcashtab-tests
Build 61444: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sat, Nov 2, 14:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Nov 2, 14:36
bytesofman edited the test plan for this revision. (Show Details)Sat, Nov 2, 14:37
Harbormaster completed remote builds in B30971: Diff 50619.Sat, Nov 2, 14:40
bytesofman published this revision for review.Sat, Nov 2, 14:46
emack accepted this revision.Sun, Nov 3, 11:18
emack added a subscriber: emack.

Have verified the merging of orderbooks for the same token offer, though I wasn't getting a duplicate key error in the first place on master (v2.49.1).

This revision is now accepted and ready to land.Sun, Nov 3, 11:18
emack requested changes to this revision.Sun, Nov 3, 11:24

actually, after you've listed two offers, verified they render as one merged orderbook, then I've noticed:

  • you need to cancel twice on the same orderbook, is this because it's representative of 2 offers?
  • the orderbook in manage listings become a "Buy token", which makes sense since you no longer have any offers left, but shouldn't it not be displayed at all?
This revision now requires changes to proceed.Sun, Nov 3, 11:24
bytesofman requested review of this revision.Sun, Nov 3, 12:52

actually, after you've listed two offers, verified they render as one merged orderbook, then I've noticed:

  • you need to cancel twice on the same orderbook, is this because it's representative of 2 offers?

Yes, each bar represents one AgoraPartial. So, you can cancel individual offers. We do not have a feature for "cancel all my offers this orderbook" -- anyway, this would require a tx to cancel each offer.

  • the orderbook in manage listings become a "Buy token", which makes sense since you no longer have any offers left, but shouldn't it not be displayed at all?

This should be resolved by updates in D17064, which gives the OrderBook the ability to know if it has no offers and render accordingly.

emack accepted this revision.Mon, Nov 4, 00:43
This revision is now accepted and ready to land.Mon, Nov 4, 00:43
Closed by commit rABC5b9ca8ccad18: [Cashtab] Patch bug in manage agora offers (authored by bytesofman). · Explain WhyMon, Nov 4, 17:24
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABC5b9ca8ccad18: [Cashtab] Patch bug in manage agora offers.

Revision Contents
Changeset List

PathSize
cashtab/
src/
components/
Agora/
9 lines

Diff 50619

View Options

cashtab/src/components/Agora/index.js

Loading...