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

Mock end-to-end Unipig #409

Merged
merged 28 commits into from Sep 4, 2019

Conversation

@karlfloersch
Copy link
Contributor

commented Aug 29, 2019

Description

This PR adds an end-to-end mock unipig with a rollup chain example project. It's enormous--sorry! I'll write up a more comprehensive description shortly to make it somewhat more manageable to review.

Questions

  • TODO

Metadata

Fixes

Contributing Agreement

@karlfloersch karlfloersch force-pushed the karlfloersch:feat/mock_wallet branch from 3a3ec22 to 22ffede Aug 31, 2019
@karlfloersch karlfloersch force-pushed the karlfloersch:feat/mock_wallet branch from 5789165 to 1439493 Sep 2, 2019
@karlfloersch karlfloersch force-pushed the karlfloersch:feat/mock_wallet branch from c144717 to 609a4d2 Sep 2, 2019
@karlfloersch karlfloersch changed the title [WIP] Mock Uni OVM API Mock Unipig Sep 3, 2019
@karlfloersch karlfloersch changed the title Mock Unipig Mock end-to-end Unipig Sep 3, 2019
@karlfloersch karlfloersch added ovm @pigi and removed ovm labels Sep 3, 2019
@karlfloersch karlfloersch force-pushed the karlfloersch:feat/mock_wallet branch from 9e459f8 to 70ce92a Sep 3, 2019
Copy link
Collaborator

left a comment

Looks good! Left a lot of small comments, but I'm in favor of merging 👍

}

/* Listeners */
setTimeout(() => {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

Since the HTML elements are static, you should be able to declare these event handlers in the HTML element so you don't have to set an arbitrary timeout time to bind them.


<script>
// Set some functions that we will use to change the ui
window.updateAccountAddress = function(address) {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

I think this and the other functions are redundant with the functions in index.ts

/*
* Get balances for some account
*/
[AGGREGATOR_API.getBalances]: (account: Address): Balances =>

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

If SimpleServer is adjusted to be able to take a class (or simply parse public functions from a class), we can:

  1. Define AGGREGATOR_API to be an interface instead of a map of func names
  2. Pass an instance of that interface instead of creating these dummy name => function mappings

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 4, 2019

Author Contributor

Ah nice this is a great idea! I think that in order to get this merged more quickly I'll skip doing this for now, but I'll also add an issue for it to not forget

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 4, 2019

Collaborator

Yep, this is about the lowest priority possible. Just mentioning.

* TODO: replace sign(...) with a reference to the keystore.
*/
constructor(
readonly db: KeyValueStore,

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

db should be private to prevent anyone with an instance from modifying data.

* Connects to an aggregator using a provided rpcClient
* @param rpcClient the rpcClient to use -- normally it's a SimpleClient
*/
public connect(rpcClient: RpcClient) {

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

Is there any actual connecting to the client to be done here?

this.state[UNISWAP_ADDRESS].balances,
] = this.getPostSwapBalances(
swap,
this.state[sender].balances,

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

this.getPostSwapBalances should have access to this.state and UNISWAP_ADDRESS, so you should just be able to pass swap and sender and modify the this.state objects in that function instead of returning new objects and overwriting balances

export type Address = string

export interface Balances {
[tokenType: string]: number

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

I think you can make this type TokenType to restrict keys to valid TokenType strings

}

export interface Swap {
tokenType: UniTokenType | PigiTokenType

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

Can use TokenType

this.rollup = new MockRollupClient(rollupBucket, this.sign)

// Save a reference to our db
this.db = db

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

can just declare it as private db in the constructor


public async sign(address: string, message: string): Promise<string> {
// Mock the signature for now
return address

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 3, 2019

Collaborator

Maybe inject a SignatureProvider in the constructor, and then this doesn't have to be mocked here? Can always inject a mocked SignatureProvider for the same outcome.

@willmeister willmeister merged commit b19da5d into plasma-group:master Sep 4, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.