Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve publicAuction.js #1482

Closed
dtribble opened this issue Aug 15, 2020 · 2 comments · Fixed by #1562
Closed

Improve publicAuction.js #1482

dtribble opened this issue Aug 15, 2020 · 2 comments · Fixed by #1562
Assignees
Labels
hygiene Tidying up around the house Zoe package: Zoe

Comments

@dtribble
Copy link
Member

What is the Problem Being Solved?

publicAuction was written to much earlier APIs, and so is not providing good example of current API usage.

  • The code is too complicated for the function it's providing (121 lines of code)
  • It's working too hard to check things it shouldn't have to care about
  • The termination is based on number of bids, which is not part of any interesting use case
  • auction.js has helper functions just for it. Since htey are specialized to particular Keywords, they should just be private in the module with publicAuction.

Description of the Design

  • Redo using the affordances of the new APIS
  • Offer checking should rely on Zoe or use helpers
  • Termination should at the very least be "whenever the seller closes the auction" since that is at least simpler.
  • Add notifiers?
  • It could incrementally update the auction state on each bid

Security Considerations
The refundAmount logic is a hazard and should not be necessary. It's implied by the transfer, and so linear operations would be better (e.g., amount.split).

Test Plan
Tests should stay the same except that auction-close should use a better mechanism

@dtribble dtribble added enhancement New feature or request hygiene Tidying up around the house Zoe package: Zoe and removed enhancement New feature or request labels Aug 15, 2020
@katelynsills
Copy link
Contributor

@katelynsills
Copy link
Contributor

@dtribble: I didn't understand this part: The refundAmount logic is a hazard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hygiene Tidying up around the house Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants