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

[BatchExchangeViewer] allow to fetch balances #540

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fleupold
Copy link
Contributor

This PR adds a method to get all balances of tokens listed in the exchange for a given user (this excludes pathological tokens which the user deposited without listing them).

It uses the experimental ABI encoder which allows nicely structured return values (instead of returning bytes or implicitly aligned arrays).

Unfortunately we cannot give accurate estimates of how much of a token is actually withdrawable (we have to take the requested amount which could be infinitely high cf. #539)

Test Plan

unit test + deploying this on rinkeby and running:

await instance.getBalances("0x42b7e89374BcEAb213c699488f667c7a9144C28b")

Copy link

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

test/stablex/batch_exchange_viewer.js Show resolved Hide resolved
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! My only concern is the Rust support for ABI v2.

@@ -1,6 +1,8 @@
pragma solidity ^0.5.0;
pragma experimental ABIEncoderV2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know if this is supported by current versions of web3 and ethabi and hence ethcontract. I’ll have to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know. If it's supported I'd love to refactor the orderbook fetching to also return list of structs instead of byte arrays (only in the Viewer contract though as we don't want experimental features in the core contract).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead and merge this now. As we are not depending on it yet it shouldn't cause any incompatibilities and we can change it later on if we need a different encoding for ethcontracts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any news on the support check @nlordell ?

Copy link
Contributor

@nlordell nlordell Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to update here. No this is currently not supported by either web3 crate or ethcontract 😢

That being said, adding support is feasible. I can take a look at doing this later in the week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean even the other methods cannot be deserialized or only the ones that would be returning a struct? I don't know if ABIEncoderV2 changes the encoding for "traditional" return types as well.

@fleupold fleupold changed the base branch from orderbook_filter_symbold to master February 13, 2020 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants