Skip to content

Bugfix: update exchange rates#104

Merged
estebanmino merged 1 commit intomasterfrom
bufix/update-exchange-rate
May 24, 2019
Merged

Bugfix: update exchange rates#104
estebanmino merged 1 commit intomasterfrom
bufix/update-exchange-rate

Conversation

@estebanmino
Copy link
Copy Markdown
Contributor

@estebanmino estebanmino commented May 24, 2019

This PR adds a semaphore to updateExchangeRate, this was causing wrong conversion rates.
If the app sets native currency as ETH and then DAI, fetchExchangeRate could finish first for DAI, producing that ETH conversion rate is set to state because it finished later, even though it was called first.

@estebanmino estebanmino requested review from bitpshr and brunobar79 May 24, 2019 20:36
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 24, 2019

Codecov Report

Merging #104 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #104   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     21           
  Lines        1420   1424    +4     
  Branches      182    182           
=====================================
+ Hits         1420   1424    +4
Impacted Files Coverage Δ
src/CurrencyRateController.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 782b8f1...c10a8e4. Read the comment docs.

Copy link
Copy Markdown
Contributor

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@estebanmino estebanmino merged commit 94b9224 into master May 24, 2019
@whymarrh whymarrh deleted the bufix/update-exchange-rate branch November 5, 2019 17:44
mcmire pushed a commit to mcmire/core that referenced this pull request Jul 17, 2023
* 5.0.3

* Update CHANGELOG.md

* Update CHANGELOG.md and change to major version

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: david0xd <david@timechaser.org>
kanthesha pushed a commit that referenced this pull request Oct 11, 2023
This PR adds [JSON-RPC 2.0](https://www.jsonrpc.org/specification)-compliant notification handling for `JsonRpcEngine`.

- JSON-RPC notifications are defined as JSON-RPC request objects without an `id` property.
- A new constructor parameter, `notificationHandler`, is introduced. This parameter is a function that accepts JSON-RPC notification objects and returns `void | Promise<void>`.
- When `JsonRpcEngine.handle` is called, if a `notificationHandler` exists, any request objects duck-typed as notifications will be handled as such. This means that:
  - Validation errors that occur after duck-typing will be ignored. At the moment, this just means that no error will be thrown if the `method` field is not a string.
  - If basic validation succeeds, the notification object will be passed to the handler function without touching the middleware stack.
  - The response from `handle()` will be `undefined`.
  - No error will be returned or thrown, unless the notification handler itself throws or rejects.
    - Notification handlers should not throw or reject, and it is the implementer's responsibility to ensure that they do not.
- If `JsonRpcEngine.handle` is called and no `notificationHandler` exists, notifications will be treated just like requests. This is the current behavior.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
Mrtenz pushed a commit that referenced this pull request Oct 16, 2025
This is a performance improvement as the latter is actually faster.

json-stable-stringify x 13,870 ops/sec ±0.72% (94 runs sampled)
safe-stable-stringify x 30,367 ops/sec ±0.39% (96 runs sampled)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants