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

Add ABI serialization to wallet #449

Merged

Conversation

@willmeister
Copy link
Collaborator

commented Sep 18, 2019

Adds ABI serialization to wallet functionality

Also adds logging utilities

Metadata

Fixes

Contributing Agreement

willmeister added 8 commits Sep 17, 2019
- Moved rollup serialization out of core because it's not useful outside of wallet stuff
- Added rollup serialization encode / decode utility functions in place of ABI classes
- Changed uni and pigi token types to be numbers
… unit tests
…ytes32 issue for keccak output
Copy link
Contributor

left a comment

This is super dope! I left a few comments, the main one being the authentication for the faucet transactions I don't think is correct. Other than that though this is really shaping up! Awesome! Exciting! Fun! 😁

packages/example-rollup/index.ts Show resolved Hide resolved
updateBalances(
response.transactionReceipt.updatedState[wallet.address].balances
)
updateBalances(response[0].stateReceipt.state.balances)

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 19, 2019

Contributor

So I don't think we should expose state receipts to the application developer. They should just get back an object with their current balances & not even an aggregator signature. All that stuff is handled in the background (by the OVM / client) -- stuff we talked about today

This comment has been minimized.

Copy link
@willmeister

willmeister Sep 19, 2019

Author Collaborator

To be clear, you mean that we shouldn’t call updateBalances(...) here? The current API only returns signed receipts, so I’m not sure how we’d change this. I’m sure we’ll add other API endpoints for communicating with our application. Is that what you mean? That there should be another endpoint that simplifies the returned data? Either way, it seems strange to send a Tx and not get a signed receipt. Let’s talk about it today.

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 19, 2019

Contributor

Ah I thought that this was the getBalances() query for some reason I think. In this case I do agree that it makes sense to get back a signed receipt. I think this is good & long term we'll figure out the right format for these responses which I'd guess still contain a signed receipt.

At a high level my thought process is simply to hide some of the inner-workings of layer 2 solutions from application developers. For instance, I don't think anyone should learn about merkle trees if they are just a video game dev. But that doesn't mean that we don't respond with an object which contains that more fine-grained / low level info (like an http request for instance)

updateUniswapBalances(
response.transactionReceipt.updatedState[UNISWAP_ADDRESS].balances
)
updateBalances(response[0].stateReceipt.state.balances)

This comment has been minimized.

Copy link
@karlfloersch

karlfloersch Sep 19, 2019

Contributor

Once again imo we should hide the actual state receipt from the user

packages/wallet/src/utils.ts Outdated Show resolved Hide resolved
packages/example-rollup/src/mock-aggregator.ts Outdated Show resolved Hide resolved
packages/wallet/src/rollup-client.ts Show resolved Hide resolved
packages/wallet/src/types/types.ts Show resolved Hide resolved
packages/wallet/src/rollup-aggregator.ts Show resolved Hide resolved
willmeister added 3 commits Sep 19, 2019
@willmeister willmeister merged commit 1c10b3f into plasma-group:master Sep 19, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willmeister willmeister deleted the willmeister:feat/447/AddAbiSerializationToWallet branch Sep 19, 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.