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

Add getOrdersInfo function #857

Merged
merged 3 commits into from Jul 17, 2018

Conversation

Projects
None yet
5 participants
@abandeali1
Copy link
Member

abandeali1 commented Jul 11, 2018

Description

This doesn't add a ton of functionality, but allows us to easily validate a batch of orders with a single RPC call.

TODO: Add tests

Testing instructions

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Change requires a change to the documentation.
  • Added tests to cover my changes.
  • Added new entries to the relevant CHANGELOG.jsons.
  • Labeled this PR with the 'WIP' label if it is a work in progress.
  • Labeled this PR with the labels corresponding to the changed package.

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch 2 times, most recently Jul 11, 2018

@abandeali1 abandeali1 requested review from dekz and hysz Jul 11, 2018

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch Jul 11, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 11, 2018

Coverage Status

Coverage decreased (-0.03%) to 83.826% when pulling 47080ecaf887e9ca855552e49a314df5dc0b6a1a on feature/contracts/batchGetOrderInfo into 6bdee26 on v2-prototype.

@dekz

dekz approved these changes Jul 11, 2018

@hysz

hysz approved these changes Jul 12, 2018

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch 2 times, most recently Jul 13, 2018

@abandeali1 abandeali1 changed the title [WIP] Add getOrdersInfo function Add getOrdersInfo function Jul 13, 2018

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch Jul 13, 2018

packages/contracts/test/exchange/core.ts Outdated
expect(orderInfo.orderStatus).to.equal(expectedOrderStatus);
});
it('should return the correct orderInfo for an expired and unfilled order', async () => {
await increaseTimeAndMineBlockAsync(signedOrder.expirationTimeSeconds.toNumber());

This comment has been minimized.

@albrow

albrow Jul 16, 2018

Member

expirationTimeSeconds is a timestamp, denoted as milliseconds since the unix epoch, whereas increaseTimeAndMineBlockAsync expects a duration in seconds. I think what you meant to do was this:

const timeOffset = signedOrder.expirationTimeSeconds
    .sub(Date.now())
    .divToInt(1000)
    .toNumber();
await increaseTimeAndMineBlockAsync(timeOffset);

This comment has been minimized.

@abandeali1

abandeali1 Jul 16, 2018

Member

It was somewhat lazy, but increasing the time by the actual expiration timestamp does guarantee that the order will be expired and also shouldn't affect anything else.

This comment has been minimized.

@albrow

albrow Jul 16, 2018

Member

There is no way to reverse a time offset and there is a limit to the total time offset that can be applied, so it actually can matter if the offset is way too big. (We're talking about ~1.5 trillion seconds when it should be ~600 seconds, so it really is a huge difference).

In addition, if we're trying to test that order expiration is working correctly, we should be testing that as accurately as possible. The expiration time goes through a lot of conversions and is not hard to imagine that there could be a bug somewhere (e.g. we forgot to convert seconds to milliseconds). As written, the test would pass even if there was a bug causing the expiration time to be off by 10 orders of magnitude.

I'm okay with adding a small buffer here, but not one that is this big.

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch Jul 17, 2018

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch 3 times, most recently Jul 17, 2018

@abandeali1 abandeali1 force-pushed the feature/contracts/batchGetOrderInfo branch to caa5b4e Jul 17, 2018

@abandeali1 abandeali1 merged commit 56a4a37 into v2-prototype Jul 17, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: static-tests Your tests passed on CircleCI!
Details
ci/circleci: submit-coverage Your tests passed on CircleCI!
Details
ci/circleci: test-contracts-ganache Your tests passed on CircleCI!
Details
ci/circleci: test-contracts-geth Your tests passed on CircleCI!
Details
ci/circleci: test-rest Your tests passed on CircleCI!
Details

@abandeali1 abandeali1 deleted the feature/contracts/batchGetOrderInfo branch Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment