Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

(CRO-53) Balance tracking and synchronisation #25

Merged
merged 10 commits into from
Apr 1, 2019
Merged

(CRO-53) Balance tracking and synchronisation #25

merged 10 commits into from
Apr 1, 2019

Conversation

devashishdxt
Copy link
Collaborator

No description provided.

@devashishdxt
Copy link
Collaborator Author

This is a basic implementation of balance tracking. Chain trait is not implemented in this pull request. The implementation of Chain trait will involve querying Crypto.com Chain full nodes through ABCI.

@devashishdxt devashishdxt marked this pull request as ready for review April 1, 2019 04:02
@devashishdxt devashishdxt requested a review from tomtau April 1, 2019 04:02
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

I'm fine with merging in PR, but as it mainly contains hollow abstractions that are being built up for some implementation, it'll be good to document their overall aim with some references to the intended usage -- this service abstracts over a storage layer for retrieving and permitting this information, initially backed by key-value db... this service abstracts over communication with Tendermint over JSON-RPC...

client-core/src/wallet.rs Show resolved Hide resolved
client-core/src/wallet.rs Show resolved Hide resolved
@tomtau
Copy link
Contributor

tomtau commented Apr 1, 2019

@devashishdxt
Copy link
Collaborator Author

I have added README.md in client-core which explains the overall design of client-core and also the role and responsibilities of each trait and struct.

@devashishdxt devashishdxt requested a review from tomtau April 1, 2019 05:55
@devashishdxt
Copy link
Collaborator Author

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

The client-core README is pretty good!

would it be possible to make it compile with nightly: https://travis-ci.org/crypto-com/chain/jobs/513972066#L783 ?

If it's just the problem of the current nightly (and will disappear in the next release), it's ok to merge; if it's a persistent change in nightly that will stay in near future releases, it'll be good to fix it

@devashishdxt
Copy link
Collaborator Author

Nightly build failure is because of a bug introduced in latest nightly: rust-lang/rust#59553

This is a regression introduced by this pull request: rust-lang/rust#59421

@tomtau tomtau merged commit e60b90f into crypto-com:master Apr 1, 2019
@devashishdxt devashishdxt deleted the transaction-storage branch April 1, 2019 08:05
@Centril
Copy link

Centril commented Apr 1, 2019

Nightly build failure is because of a bug introduced in latest nightly: rust-lang/rust#59553

Note: The PR referenced fixed a bug introduced in some versions prior.

@estebank
Copy link

estebank commented Apr 1, 2019

@devashishdxt the new behavior is the correct behavior and used to be the active behavior until a couple of releases ago before a parser refactor introduced the incorrect behavior. We might introduce a full release cycle of warnings in beta and stable for the new behavior depending on how extensive the fallout is. If you have reasons to believe this is an incorrect course of action, please voice them in the regression issue.

@devashishdxt
Copy link
Collaborator Author

@estebank I believe new behaviour is the correct behaviour. But, we’ll have to wait until changes are made in quote or in crates using it to fix our build.

@estebank
Copy link

estebank commented Apr 1, 2019

Quote doesn't need to change. You need to wrap your integer loop using string interpolation with an ident so that quote doesn't interpret it as a number. That will fix your build and is backwards and forwards compatible.

@devashishdxt
Copy link
Collaborator Author

@estebank Our build fails because, one of our dependency, zeroize_derive, exposes a derive macro which internally uses quote. So, to fix this issue, one of them (quote or zeroize_derive) will have to make some changes. Please let me know if I’m missing something and if we can make any changes in our code to fix this.

@estebank
Copy link

estebank commented Apr 1, 2019

I see. The fix will have to happen in zeroize_derive.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants