Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Add decoders for broker and stop-limit data #2484

Merged
merged 3 commits into from
Feb 15, 2020

Conversation

moodlezoup
Copy link
Contributor

Description

Adds decoding equivalents to the encoders found in chainlink_utils and gods_unchained_utils

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@moodlezoup moodlezoup force-pushed the broker-and-stop-limit-data-decoders branch from 78754dc to 8371bc8 Compare February 13, 2020 23:31
@coveralls
Copy link

coveralls commented Feb 13, 2020

Coverage Status

Coverage increased (+0.08%) to 79.739% when pulling bd22263 on broker-and-stop-limit-data-decoders into a4ac418 on development.

Copy link
Contributor

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

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

preesh the encoding/decoding tests!

Comment on lines 6 to 7
proto: BigNumber;
quality: BigNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

would the abi encoder accept these if they were just numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, changed to BigNumber | number

Comment on lines 717 to 718
const MAX_UINT8 = new BigNumber(2).exponentiatedBy(8).minus(1);
const MAX_UINT16 = new BigNumber(2).exponentiatedBy(16).minus(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Seems clearer to just do like new BigNumber(2**8-1)

@@ -15,12 +27,13 @@ import { LocalBalanceStore } from '../framework/balances/local_balance_store';
import { DeploymentManager } from '../framework/deployment_manager';
import { ChainlinkStopLimitContract, TestChainlinkAggregatorContract } from '../wrappers';

blockchainTests.resets('Chainlink stop-limit order tests', env => {
blockchainTests.resets.only('Chainlink stop-limit order tests', env => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
blockchainTests.resets.only('Chainlink stop-limit order tests', env => {
blockchainTests.resets('Chainlink stop-limit order tests', env => {

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

LGTM! 🥂

@moodlezoup moodlezoup merged commit dcce827 into development Feb 15, 2020
@moodlezoup moodlezoup deleted the broker-and-stop-limit-data-decoders branch February 15, 2020 01:38
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.

4 participants