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

703 channel provider tests #708

Merged
merged 72 commits into from Jan 14, 2020
Merged

703 channel provider tests #708

merged 72 commits into from Jan 14, 2020

Conversation

@pedrouid
Copy link
Member

pedrouid commented Jan 10, 2020

The Problem

The Solution

Checklist:

  • I am making this PR against staging, not master
  • My code follows the code style of this project.
  • I have described any backwards-incompatibility implications above.
  • I have highlighted which parts of the code should be reviewed most carefully.
  • If my change requires a change to the documentation, I have updated it accordingly.
@todo-tracker

This comment has been minimized.

Copy link

todo-tracker bot commented Jan 11, 2020

Hey, pedrouid

We noticed you made changes to a file with a TODO on it.
These are set to make sure potential Technical Debt doesn't get forgotten.
While you're here take a shot at turning a listed TODO into a TODONE!

Id Name File Priority
66 determine virtual app in a more resilient way modules/client/src/listener.ts Normal
71 merge: modules/client/src/node.ts Normal
78 why is it sometimes data vs data.data? modules/client/src/controllers/AbstractController.ts Normal

button

@todo-tracker

This comment has been minimized.

Copy link

todo-tracker bot commented Jan 11, 2020

Hey, pedrouid

It looks like you made some changes to this pull request!
These changes included files that have a TODO linked to them.
Click the button below to view details on all the TODO for this pull request, and while you're here take a shot at turning a TODO into a TODONE!

Id Name File Priority
64 expose remove listener modules/node/src/transfer/transfer.service.ts Normal
68 rename to "get" when in branch to publish new client modules/node/src/transfer/transfer.provider.ts Normal
69 import from connext/types modules/dashboard/src/lib/messaging.js Normal
83 should add once to top level client modules/test-runner/src/util/helpers/fundChannel.ts Normal
84 Currently, this test always fails because when promise is never rejected when modules/test-runner/src/swap/swap.test.ts Normal
85 events for withdrawal commitments! issue 698 modules/test-runner/src/withdraw/withdraw.test.ts Normal

button

pedrouid added 4 commits Jan 11, 2020
@pedrouid

This comment has been minimized.

Copy link
Member Author

pedrouid commented Jan 11, 2020

Fix #703

pedrouid added 5 commits Jan 13, 2020
move InternalClientOptions and NodeInitializationParameters to types module
Copy link
Contributor

LayneHaber left a comment

This all has a lot of good cleanup in it, well done!

Tentatively approving, but shouldn't be merged until all tests are passing

modules/client/src/channelProvider.ts Outdated Show resolved Hide resolved
modules/client/src/channelProvider.ts Outdated Show resolved Hide resolved
modules/client/src/channelProvider.ts Outdated Show resolved Hide resolved
modules/client/src/channelProvider.ts Outdated Show resolved Hide resolved
this.connection.on(event, listener);
return this.connection;
};

public once = (event: string, listener: (...args: any[]) => void): RpcConnection => {
public once = (event: string, listener: (...args: any[]) => void): any => {

This comment has been minimized.

Copy link
@LayneHaber

LayneHaber Jan 13, 2020

Contributor

we can do a better job of the event and return types here as well probably

This comment has been minimized.

Copy link
@pedrouid

pedrouid Jan 13, 2020

Author Member

Actually this could just be void

This comment has been minimized.

Copy link
@pedrouid

pedrouid Jan 13, 2020

Author Member

Actually it can't because it extends from EventEmitter

export const calculateExchange = (amount: BigNumber, swapRate: string): BigNumber => {
return bigNumberify(formatEther(amount.mul(parseEther(swapRate))).replace(/\.[0-9]*$/, ""));
};
Comment on lines +7 to +9

This comment has been minimized.

Copy link
@LayneHaber

LayneHaber Jan 13, 2020

Contributor

isn't this a function exported from the client? seems like we should use that (but don't have a huge pref here, i think we just have the same fn in three places: here, client, daicard)

This comment has been minimized.

Copy link
@pedrouid

pedrouid Jan 13, 2020

Author Member

not sure here just used what was available before in the swap.test.ts cc @rhlsthrm

modules/test-runner/src/util/channelProvider.ts Outdated Show resolved Hide resolved
modules/test-runner/src/util/client.ts Show resolved Hide resolved
export type JsonRpcRequest = {
id: number;
jsonrpc: "2.0";
method: string;
params: any;
};
Comment on lines +87 to +92

This comment has been minimized.

Copy link
@LayneHaber

LayneHaber Jan 13, 2020

Contributor

is this exported from cf or no?

This comment has been minimized.

Copy link
@pedrouid

pedrouid Jan 13, 2020

Author Member

vscode wasn't returning anything but I can search again

This comment has been minimized.

Copy link
@pedrouid

pedrouid Jan 13, 2020

Author Member

Actually it doesn't because it uses a different schema

export type Rpc = {
  methodName: string;
  parameters: RpcParameters;
  id?: number;
};
@todo-tracker

This comment has been minimized.

Copy link

todo-tracker bot commented Jan 14, 2020

Hey, pedrouid

It looks like you made some changes to this pull request!
These changes included files that have a TODO linked to them.
Click the button below to view details on all the TODO for this pull request, and while you're here take a shot at turning a TODO into a TODONE!

Id Name File Priority
70 make this actually activate wallet connect cypress/tests/utils.js Normal

button

pedrouid added 7 commits Jan 14, 2020
@rhlsthrm rhlsthrm merged commit da442e9 into staging Jan 14, 2020
13 checks passed
13 checks passed
build
Details
test-cf
Details
test-cf
Details
test-client
Details
test-client
Details
test-contracts
Details
test-contracts
Details
test-integration
Details
test-node
Details
test-integration
Details
test-backwards-compatibility
Details
test-bot
Details
test-daicard
Details
@rhlsthrm rhlsthrm deleted the 703-channelProvider-tests branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.