Skip to content
This repository has been archived by the owner on Jun 2, 2019. It is now read-only.

CoinTicker should be Realm object. #493

Closed
OlegGordiichuk opened this issue Mar 16, 2018 · 33 comments
Closed

CoinTicker should be Realm object. #493

OlegGordiichuk opened this issue Mar 16, 2018 · 33 comments
Labels
enhancement Gitcoin Gitcoin is the easiest way to monetize or incentivize work in Open Source Software. help wanted high priority

Comments

@OlegGordiichuk
Copy link
Contributor

OlegGordiichuk commented Mar 16, 2018

Also we should have relationship between CoinTicker and TokenObject.

TokenObject is an ERC20 object that contains information about specific token and also current balance of the user.
CoinTicker coinmarket ticker that contains price information for the specified token. This need to be combined into TokenObject so you can easily get pricing and later sort TokenObject by total value in the tokens list.

Keep in mind, there is different currencies available.

@gitcoinbot
Copy link

This issue now has a funding of 0.05 ETH (27.92 USD @ $558.46/ETH) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $3616.3 more Funded OSS Work Available at: https://gitcoin.co/explorer

@maektwain
Copy link

Please provide little bit description and directions.

@vikmeup
Copy link
Contributor

vikmeup commented Mar 21, 2018

@maektwain updated the issue, please take a look.

@OlegGordiichuk OlegGordiichuk added the Gitcoin Gitcoin is the easiest way to monetize or incentivize work in Open Source Software. label Mar 21, 2018
@DarthMike
Copy link
Contributor

I understand you want to integrate CoinTicker into the Realm datastore as a relationship to the TokenObject ?

If so, should the ticker be called CoinTickerObject?

I think I will attempt this task and come with a PR during weekend.

@vikmeup
Copy link
Contributor

vikmeup commented Mar 24, 2018

@DarthMike thanks for doing this!

I think CoinTicker is great! I think before we had one model, we called them CoinTicker and CoinTickerObject for the realm model.

Currently, we store tickers object into NSUserDefaults, kinda sucks. But also make sure to use currency parameter to distinguish.
Also, each network has separate realm file, so you don't need to worry about that.

Do you have telegram? can you message me at vikmeup?

@vikmeup
Copy link
Contributor

vikmeup commented Mar 24, 2018

Yes, so currently the way it's done:
We have TokenObject then we ask viewModel for getTicker(for: TokenObject), but instead we could have some mapping

struct TokenObject {
    var price: CoinTicker
}

unless you know better alternatives? always up for it

@DarthMike
Copy link
Contributor

Something has come up, won't be able to work on this for a while. Just letting you know so if somebody else wants to, they can pick it up.

@johnnynanjiang
Copy link
Contributor

@vikmeup I’m looking at this issue if nobody else is.

@vikmeup
Copy link
Contributor

vikmeup commented Apr 6, 2018

@johnnynanjiang nice! thank you

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 6, 2018
- fix a discrepency in TokensDataStore
- add an unit test for TokensDataStore
@vs77bb
Copy link

vs77bb commented Apr 7, 2018

@johnnynanjiang Feel free to pick this up on Gitcoin here by clicking 'Start Work'!

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 8, 2018
- add CoinTickerObject Realm object
- switch from UserDefaults to Realm for tickers
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 8, 2018
- refactor and update unit test
@gitcoinbot
Copy link

gitcoinbot commented Apr 8, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

  1. @johnnynanjiang

has committed to working on this project to be completed 10 months from now.

, please see the below comments / questions regarding approach for this ticket from the bounty hunter(s):

@johnnynanjiang
Copy link
Contributor

Thanks @vs77bb just done it

@johnnynanjiang
Copy link
Contributor

Hi @vikmeup and @OlegGordiichuk, just wanted to clarify some points and get your insights.

I've made some changes so far so that CoinTicker now gets saved and retrieved from Realm (via CoinTickerObject, with a tickerskey for distinguishing currency and network. CoinTickerObject will eventually replace CoinTicker)

Now I'm looking at combining TokenObject and CoinTicker

TokenObject {
    var price: CoinTicker
}

Some options as follows:

  1. Combine TokenObject and CoinTicker from service side.
    Currently, we get TokenObject and CoinTicker from two separate endpoints .getTokens and .prices (there is another RPC endpoint tokenBalance), probably we could combine them in .getTokens so that when we call .getTokens we will have TokenObject and CoinTicker details returned in the response.

  2. Combine TokenObject and CoinTicker from mobile side
    We get TokenObject from .getTokens, and get CoinTicker from .prices, then combine CoinTicker into TokenObject.

  3. Leave TokenObject and CoinTicker separate in Realm, as they are from separate endpoints anyway.

Which way do you prefer or if I miss or misunderstand anything here?

Thanks and regards

@vikmeup
Copy link
Contributor

vikmeup commented Apr 9, 2018

@johnnynanjiang I would go with option #3, I think balance and pricing should be two separate entities in general.
option #2 might work as well, but we need to make sure they could work async as well. Once I get balance it should be presented to the user and pricing comes later and update it.

I'm also open to suggestions if you have ideas on how to simplify it.

@OlegGordiichuk did you have anything in mind on this?

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 10, 2018
- replace CoinTicker with CoinTickerObject
@johnnynanjiang
Copy link
Contributor

@vikmeup yeah it makes sense, will raise a PR soon, thanks.

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 10, 2018
@vikmeup
Copy link
Contributor

vikmeup commented Apr 10, 2018

@johnnynanjiang another reason why we need this change to sort tokens by total value. balance * token price. Can you make sure that's possible?

@johnnynanjiang
Copy link
Contributor

@vikmeup ok, I guess the idea is that there would be a new field called 'balance' in TokenObject, whenever TokenObject list or CoinTickerObject list get updated in Realm, 'balance' will get updated to 'token amount (from TokenObject) * token price (from CoinTickerObject)' in TokenObject.

How does it sound?

@vikmeup
Copy link
Contributor

vikmeup commented Apr 11, 2018

I think that make sense to me to have balance field on TokenObject.

I think this logic should be simplified, we could have pricing storage as it is and just update balance when pricing changes.

That’s not what original issue states tho, but that’s ok! We just need good solution.

Can you telegram me: vikmeup?

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 12, 2018
- update balance whenever token, ticker or balance get updated
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 12, 2018
- update wording to camel case
@johnnynanjiang
Copy link
Contributor

@vikmeup sure, I have implemented the logic in TokensDataStore as it is where we store/update TokenObject, CoinTickerObject, and Balance. Testing at the moment, as well as implementing Realm migration as new field 'balance' has been added.

@johnnynanjiang
Copy link
Contributor

@gitcoinbot yes, submitted a PR and there is some work to be done.

@gitcoinbot
Copy link

@johnnynanjiang. 👋 thanks for the atMention, but you need to install @gitcoinbot on this repo for me to be able to respond. More details in the documentation.

✌️
@gitcoinbot

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 19, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 19, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 19, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 19, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 20, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 21, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 21, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 21, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 21, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 21, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 22, 2018
vikmeup pushed a commit that referenced this issue Apr 22, 2018
* Bug/493 (#493)

- fix a discrepency in TokensDataStore
- add an unit test for TokensDataStore

* Bug/493 (#493)

- add CoinTickerObject Realm object
- switch from UserDefaults to Realm for tickers

* Bug/493 (#493)

- refactor and update unit test

* Bug/493 (#493)

- replace CoinTicker with CoinTickerObject

* Bug/493 (#493)

- fix test

* Bug/493 (#493)

- update balance whenever token, ticker or balance get updated

* Bug/493 (#493)

- update wording to camel case

* Bug/493 (#493)

- implement TokenObject data migration in Realm

* Bug/493 (#493)

- exclude field to avoid decoding error

* Bug/493 (#493)

- refactoring

* Bug/493 (#493)

- change TokenObject.balance type to Double

* Bug/493 (#493)

- fix Realm migration

* Bug/493 (#493)

- apply use of TokenObject.balance

* Bug/493 (#493)

- apply use of TokenObject.balance

* Revert "Bug/493 (#493)"

This reverts commit 1be6c79.

* Revert "Bug/493 (#493)"

This reverts commit 945c37d.

* Bug/493 (#493)

- fix unit tests

* [#493] - update cocoapods to 1.5.0

* [#493] - bump the Realm schema version to 50

* [#493] - fix Realm migration for shared data store, and refactoring

* [#493] - remove redundant code

* [#493] - remove redundant Realm migration code

* [#493] - change from realm.add back to realm.create

* [#493] - remove duplicate .map{}

* [#493] - use try? instead of try catch

* [#493] - rename CoinTickerObject back to CoinTicker

* [#493] - fix merge conflict

* [#493] - fix unit tests
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 22, 2018
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 22, 2018
vikmeup pushed a commit that referenced this issue Apr 22, 2018
* Bug/493 (#493)

- fix a discrepency in TokensDataStore
- add an unit test for TokensDataStore

* Bug/493 (#493)

- add CoinTickerObject Realm object
- switch from UserDefaults to Realm for tickers

* Bug/493 (#493)

- refactor and update unit test

* Bug/493 (#493)

- replace CoinTicker with CoinTickerObject

* Bug/493 (#493)

- fix test

* Bug/493 (#493)

- update balance whenever token, ticker or balance get updated

* Bug/493 (#493)

- update wording to camel case

* Bug/493 (#493)

- implement TokenObject data migration in Realm

* Bug/493 (#493)

- exclude field to avoid decoding error

* Bug/493 (#493)

- refactoring

* Bug/493 (#493)

- change TokenObject.balance type to Double

* Bug/493 (#493)

- fix Realm migration

* Bug/493 (#493)

- apply use of TokenObject.balance

* Bug/493 (#493)

- apply use of TokenObject.balance

* Revert "Bug/493 (#493)"

This reverts commit 1be6c79.

* Revert "Bug/493 (#493)"

This reverts commit 945c37d.

* Bug/493 (#493)

- fix unit tests

* [#493] - update cocoapods to 1.5.0

* [#493] - bump the Realm schema version to 50

* [#493] - fix Realm migration for shared data store, and refactoring

* [#493] - remove redundant code

* [#493] - remove redundant Realm migration code

* [#493] - change from realm.add back to realm.create

* [#493] - remove duplicate .map{}

* [#493] - use try? instead of try catch

* [#493] - rename CoinTickerObject back to CoinTicker

* [#493] - fix merge conflict

* [#493] - fix unit tests

* [#493] - fix realm.write()

* [#493] - replace try! with try?
johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 22, 2018
@vikmeup vikmeup closed this as completed Apr 22, 2018
@gitcoinbot
Copy link

Work for 0.05 ETH (30.31 USD @ $606.25/ETH) has been submitted by:

  1. @johnnynanjiang

Submitters, please leave a comment to let the funder (and the other parties involved) that you've submitted you work. If you don't leave a comment, the funder may expire your submission at their discretion.

johnnynanjiang added a commit to johnnynanjiang/trust-wallet-ios that referenced this issue Apr 23, 2018
@vs77bb
Copy link

vs77bb commented Apr 27, 2018

@vikmeup Mind going to the Gitcoin Issue and accepting work, assuming this one is ready for pay out? Hope you both are doing well!

@vs77bb
Copy link

vs77bb commented May 14, 2018

@vikmeup Bump to see if @johnnynanjiang should be paid for this PR which looks to be merged. Hope you are doing well!

@vikmeup
Copy link
Contributor

vikmeup commented May 14, 2018

@vs77bb I’m pretty sure I already pressed to pay, not sure why it didn’t happen yet.

@johnnynanjiang
Copy link
Contributor

johnnynanjiang commented May 15, 2018 via email

@owocki
Copy link

owocki commented May 17, 2018

@vikmeup do you have a tx id for the acceptance transaction? were not seeing it here at gitcoin

@vikmeup
Copy link
Contributor

vikmeup commented May 20, 2018

@owocki when I press "Accept", it gives me this in the console:

Syntax error, unrecognized expression: select[name=bountyFulfillment [Rollbar not enabled]

I'm doing from Trust, do you what this could be?

owocki added a commit to gitcoinco/web that referenced this issue May 21, 2018
@owocki
Copy link

owocki commented May 21, 2018

@vikmeup thanks for the heads up! i just put in a fix for this ( gitcoinco/web@e9ca613 ) and deployed it!

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 0.05 ETH (34.94 USD @ $698.88/ETH) attached to this issue has been approved & issued to @johnnynanjiang.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Gitcoin Gitcoin is the easiest way to monetize or incentivize work in Open Source Software. help wanted high priority
Projects
None yet
Development

No branches or pull requests

10 participants