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

Client-side executeTransaction(...) #429

Merged

Conversation

@willmeister
Copy link
Collaborator

commented Sep 4, 2019

Description

Really this just builds on Karl's work and adds exchange fees to swaps

Metadata

Fixes

Contributing Agreement

willmeister added 2 commits Sep 4, 2019
@karlfloersch

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Will do a full review soon but just wanted to quickly note that I think you git added an unneeded package-lock.json which is making this PR deceptively large 😁.

Copy link
Contributor

left a comment

Left a couple comments but generally looks great! Ready to merge!

@@ -28,7 +28,7 @@ export class MockRollupClient {
* TODO: replace sign(...) with a reference to the keystore.
*/
constructor(
readonly db: KeyValueStore,
private readonly db: KeyValueStore,

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

Thank you!

balances: {
uni: 0,
pigi: 0,
},

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

Ah meant to comment on this in my PR review, but one thing that we don't want to do is store anything in the state based on a getBalances() query. Basically because that means that someone could technically bloat our state by simply asking for random balances they know are zero. That's why I simply returned the value

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 5, 2019

Author Collaborator

Ahh ok. I can protect it from cases like that. Will update 👍

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 5, 2019

Author Collaborator

Ok, this should be fixed now.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

Woot!

tokenType: TokenType,
greaterThanAmount: number
) {
private hasBalance(account: Address, tokenType: TokenType, balance: number) {

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 5, 2019

Contributor

😁 I like the name

@willmeister willmeister merged commit 46d5c92 into plasma-group:master Sep 5, 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
@willmeister willmeister deleted the willmeister:feat/387/ExecuteTransaction branch Sep 5, 2019
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.