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

multi: support fiat conversion for assets #1600

Merged
merged 2 commits into from
Jul 22, 2022

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Apr 28, 2022

This enables the conversion of assets value to fiat equivalent. Configuring fiat conversion is UI only. closes #1579

  • Modify wallets page to display fiat value for the asset if fiat price is available.
  • Show fiat value for send amount on the send form.
  • Show fiat value for order amount on the order confirmation form
  • Modify the settings page to display exchange rate sources and configuration options for fiat conversion.

Wallet view
usddisplaybal

Market view
usdmk1
usdmk2
usdmk3

Withdraw/Send Form view
usdwithdrawval

Exchange rate source settings view
Screenshot from 2022-06-28 20-07-47

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Just some comments on the code. Concept looks fine to me.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/db/bolt/db.go Outdated Show resolved Hide resolved
client/db/bolt/db.go Outdated Show resolved Hide resolved
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

First pass. Lots of good stuff here.

client/webserver/api.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/markets.tmpl Outdated Show resolved Hide resolved
client/webserver/site/src/html/wallets.tmpl Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/notification.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
@chappjc chappjc modified the milestones: TBD, 0.5 Jun 21, 2022
@chappjc
Copy link
Member

chappjc commented Jun 23, 2022

No rush @ukane-philemon, just wanted to bump since the ball is in your court with current reviews, in case you missed.

@ukane-philemon
Copy link
Contributor Author

@chappjc, thanks. Will get them fixed.

@ukane-philemon ukane-philemon force-pushed the fiatconv branch 2 times, most recently from b69de9d to 8b805d3 Compare June 24, 2022 17:42
@ukane-philemon
Copy link
Contributor Author

Send Form:
sendValue

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/webserver/site/src/js/doc.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/wallets.ts Outdated Show resolved Hide resolved
client/webserver/live_test.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/wallets.tmpl Outdated Show resolved Hide resolved
client/webserver/site/src/html/markets.tmpl Outdated Show resolved Hide resolved
@ukane-philemon ukane-philemon force-pushed the fiatconv branch 2 times, most recently from 21256d6 to 2d94c2f Compare June 28, 2022 19:06
@ukane-philemon
Copy link
Contributor Author

@buck54321, I've made changes and updated the UI display in my first comment.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/types.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/markets.tmpl Outdated Show resolved Hide resolved
@ukane-philemon ukane-philemon force-pushed the fiatconv branch 2 times, most recently from a09fd81 to 6937978 Compare July 4, 2022 10:17
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Almost ready.

Comment on lines +137 to +123
if asset.Wallet == nil {
// we don't want to fetch rates for assets with no wallet.
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

We may end up wanting to get more sophisticated with this. One place the client might benefit from asset conversions is in the registration fee asset selection, which would require us to get rates for non-wallet assets.

Nothing to do here, just looking forward.

client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/wallets.tmpl Outdated Show resolved Hide resolved
client/webserver/site/src/js/app.ts Outdated Show resolved Hide resolved
client/core/types.go Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

update send form UI:
usdvform

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Currently unable to load the webserver on simnet with a panic:

panic
2022/07/06 11:11:25 http: panic serving 127.0.0.1:34718: runtime error: slice bounds out of range [-1:]
goroutine 2172 [running]:
net/http.(*conn).serve.func1()
        /usr/lib/go/src/net/http/server.go:1825 +0xbf
panic({0x141c1a0, 0xc001cae2e8})
        /usr/lib/go/src/runtime/panic.go:844 +0x258
github.com/go-chi/chi/v5/middleware.prettyStack.decorateFuncCallLine({}, {0xc000862e5b, 0x1d}, 0x4?, 0x8)
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:130 +0x545
github.com/go-chi/chi/v5/middleware.prettyStack.decorateLine({}, {0xc000862e5b?, 0xc00027a800?}, 0x90?, 0x1?)
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:106 +0x15d
github.com/go-chi/chi/v5/middleware.prettyStack.parse({}, {0xc001cfa000, 0xaf4, 0xc00222b1d8?}, {0x12fc920, 0x21a0700})
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:89 +0x4e5
github.com/go-chi/chi/v5/middleware.PrintPrettyStack({0x12fc920, 0x21a0700})
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:46 +0x45
github.com/go-chi/chi/v5/middleware.(*defaultLogEntry).Panic(0x3c?, {0x12fc920?, 0x21a0700?}, {0x8?, 0x0?, 0x0?})
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/logger.go:165 +0x29
github.com/go-chi/chi/v5/middleware.Recoverer.func1.1()
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:28 +0xcc
panic({0x12fc920, 0x21a0700})
        /usr/lib/go/src/runtime/panic.go:838 +0x207
decred.org/dcrdex/client/core.(*Core).saveDisabledRateSources(0xc0003d96c0)
        /home/joe/git/dcrdex/client/core/core.go:7689 +0x19a
decred.org/dcrdex/client/core.(*Core).fiatConversions(0xc0003d96c0)
        /home/joe/git/dcrdex/client/core/core.go:7602 +0x1a5
decred.org/dcrdex/client/core.(*Core).User(0xc0003d96c0)
        /home/joe/git/dcrdex/client/core/core.go:1749 +0x53
decred.org/dcrdex/client/webserver.(*WebServer).authMiddleware.func1({0x7f10c05291c8, 0xc001c991c0}, 0xc00160b100)
        /home/joe/git/dcrdex/client/webserver/middleware.go:43 +0x99
net/http.HandlerFunc.ServeHTTP(0x10?, {0x7f10c05291c8?, 0xc001c991c0?}, 0x4167b9?)
        /usr/lib/go/src/net/http/server.go:2084 +0x2f
github.com/go-chi/chi/v5/middleware.Recoverer.func1({0x7f10c05291c8?, 0xc001c991c0?}, 0xc0016278c0?)
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/recoverer.go:37 +0x83
net/http.HandlerFunc.ServeHTTP(0x13e86e0?, {0x7f10c05291c8?, 0xc001c991c0?}, 0xe?)
        /usr/lib/go/src/net/http/server.go:2084 +0x2f
decred.org/dcrdex/client/webserver.(*WebServer).securityMiddleware.func1({0x7f10c05291c8, 0xc001c991c0}, 0xc001ce8400?)
        /home/joe/git/dcrdex/client/webserver/middleware.go:34 +0x4ed
net/http.HandlerFunc.ServeHTTP(0xc00160b000?, {0x7f10c05291c8?, 0xc001c991c0?}, 0x203000?)
        /usr/lib/go/src/net/http/server.go:2084 +0x2f
github.com/go-chi/chi/v5/middleware.RequestLogger.func1.1({0x197e468, 0xc001cb0380}, 0xc00160b000)
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/middleware/logger.go:57 +0x3aa
net/http.HandlerFunc.ServeHTTP(0x197ef78?, {0x197e468?, 0xc001cb0380?}, 0x21a0410?)
        /usr/lib/go/src/net/http/server.go:2084 +0x2f
github.com/go-chi/chi/v5.(*Mux).ServeHTTP(0xc0001a8780, {0x197e468, 0xc001cb0380}, 0xc00160af00)
        /home/joe/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.1/mux.go:88 +0x442
net/http.serverHandler.ServeHTTP({0xc001ce8360?}, {0x197e468, 0xc001cb0380}, 0xc00160af00)
        /usr/lib/go/src/net/http/server.go:2916 +0x43b
net/http.(*conn).serve(0xc001cbe140, {0x197f020, 0xc000140a50})
        /usr/lib/go/src/net/http/server.go:1966 +0x5d7
created by net/http.(*Server).Serve
        /usr/lib/go/src/net/http/server.go:3071 +0x4db

And after that I am unable to stop dexc.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Working well for me.

client/core/core.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Test well and looks right. A few initial comments, but full review still coming.

client/core/notification.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/settings.tmpl Show resolved Hide resolved
client/webserver/site/src/html/settings.tmpl Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

We might need to revise the tooltip infomation.
Screenshot 2022-07-11 at 12 58 26 PM

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

When I enable dcrdata on simnet, navigate to the markets page, and prepare an order, the fiat rate is not shown. I have to refresh.

I can enableLogger('notes', true) and see that the notification is coming through.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jul 12, 2022

I have to refresh.

maybe the fiat rate was not available at the time you prepared the order. A refresh requests the rates directly from the client-server via fetchUser()
Edit: Fixed in 06243a5

@ukane-philemon ukane-philemon force-pushed the fiatconv branch 2 times, most recently from e53327b to 06243a5 Compare July 12, 2022 02:04
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
@ukane-philemon ukane-philemon force-pushed the fiatconv branch 4 times, most recently from 06c96ca to cd7a05a Compare July 13, 2022 15:21
@chappjc
Copy link
Member

chappjc commented Jul 14, 2022

Feel free to squash before rebasing to make resolution simpler. i.e. git rebase -i $(git merge-base master HEAD) [squash via fixup commits], then git rebase master [resolve one commit instead of 3]

client/db/interface.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/exchangeratefetcher.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
This enables the conversion of assets value to fiat equivalent.
- Modify wallets page to display fiat value for asset if asset exchange rate is available.
- Modify settings page to display available exchange rate source.
- Modify order verify form to display value for base and quote assets.
- Add new enpoint to support enabling and disabling of exchange rate source.
- Add tests to cover exchange rate fetching.
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Working well. Good stuff! A few minor nits remaining. Will wait on @buck54321 for a closer look at frontend bits.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/webserver/site/src/html/wallets.tmpl Outdated Show resolved Hide resolved
client/webserver/site/src/js/app.ts Show resolved Hide resolved
client/webserver/site/src/js/app.ts Outdated Show resolved Hide resolved
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

These should be my last requests.

client/core/notification.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Jul 22, 2022

@buck54321, @chappjc, @JoeGruffins thanks for the reviews and making this look shiny :)

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.

client/core: fiat conversions
4 participants