Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Number Precision - Use Repo.CurrencyValue to represent amounts uniformly #1666

Merged
merged 42 commits into from
Aug 28, 2019

Conversation

amangale
Copy link
Collaborator

This PR makes the changes required for bringing in repo.CurrencyValue to represent amount/value uniformly across the code.
All the wallet interface methods have also been updated to use CurrencyValue. All the wallet implementations also have been updated.

@amangale amangale requested review from placer14 and cpacia July 29, 2019 15:16
@amangale amangale self-assigned this Jul 29, 2019
@coveralls
Copy link

coveralls commented Jul 29, 2019

Coverage Status

Coverage decreased (-0.4%) to 38.091% when pulling c080927 on numprec into e225593 on master.

@drwasho drwasho added the 🚧 in progress Issue or PR is currently being worked on and not in final form. label Jul 30, 2019
@cpacia cpacia added 🔍 readyForReview Issue or PR ready for code review prior to closing. and removed 🚧 in progress Issue or PR is currently being worked on and not in final form. labels Jul 31, 2019
@drwasho
Copy link
Member

drwasho commented Aug 16, 2019

I think it would be useful to define some requirements for code review and the merge conditions; I'll check off what we have confirmed:

  • 1. Passing unit tests
    • Determine if existing unit tests cover the new code?
    • New and/or updated unit tests written (if applicable)
  • 2. Passing QA tests
    • Determine if existing QA tests cover the new code?
    • New and/or updated QA tests written (if applicable)
  • 3. Passing functional tests (on a new node, not an 'upgraded node')
    • API calls as per expected requests/responses
      • GET /wallet/balance
      • POST /ob/listing
      • POST /ob/purchase
      • POST /ob/orderspend
      • POST /ob/orderfulfillment
      • POST /ob/ordercompletion
      • GET /ob/order/:orderId
    • Create and edit listings
    • Test buying and selling a listing
    • Test various order states +/- disputes
  • 4. Passes code quality review
    • Metalinter
    • Error and state handling to avoid panics
    • Comments
  • 5. Passes acceptance criteria (TBD)

@amangale
Copy link
Collaborator Author

@placer14 @cpacia -
I have added a sample request - response transcript here: https://gist.github.com/amangale/2d91c8dc4eb95a1fdf34363b83cf901d

This is indicative of the changed responses due to the repo.CurrencyValue introduction.

@placer14 placer14 mentioned this pull request Aug 20, 2019
2 tasks
placer14 and others added 2 commits August 20, 2019 17:00
* origin/master:
  Fix 'exported method OpenBazaarNode.GetOrder should have comment or be unexported'
  Update error message in test case
  Address review notes
  Fix linting warnings
  Extracted method core.GetOrder(), added unit test for it

 Conflicts:
        api/jsonapi.go
        core/order.go
@placer14 placer14 mentioned this pull request Aug 21, 2019
7 tasks
@placer14
Copy link
Member

For others following along, we're going to stick a pin on this PR and merge it into a master-derived branch (called ethereum-master) so we can continue refining this work without blocking this request further.

@placer14 placer14 changed the base branch from master to ethereum-master August 28, 2019 23:42
@placer14 placer14 merged commit 9fc965f into ethereum-master Aug 28, 2019
@placer14 placer14 deleted the numprec branch August 28, 2019 23:43
@placer14 placer14 restored the numprec branch August 29, 2019 14:56
@placer14 placer14 deleted the numprec branch August 29, 2019 14:57
@placer14 placer14 removed the 🔍 readyForReview Issue or PR ready for code review prior to closing. label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants