-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: 85 update swap page logic to cosmjs usage #93
Conversation
nickzoum
commented
Jul 19, 2022
•
edited
Loading
edited
- Indexer provider context to hold the state (queries tendermint and updates itself by listening to the events)
- Very basic router implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one comment about context mutations that needs to be fixed. The rest are comments, questions, and suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like some changes to the environment variables
src/lib/web3/indexerProvider.tsx
Outdated
function getFullData(): Promise<PairMap> { | ||
return new Promise(function (resolve, reject) { | ||
if (!indexerURL) return reject(new Error('Undefined indexer URL')); | ||
fetch(indexerURL) | ||
.then((res) => res.json()) | ||
.then(transformData) | ||
.then(resolve) | ||
.catch(reject); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be limited by the pagination.limit
. I noted this a little in https://github.com/duality-labz/duality-web-app/pull/89/files#diff-6229acd4cf5c56748b3fe173cff7f77469038a9815260bdb22f3551091b6b6e0R97-R103 though I didn't address it there, we could do something to be a little more explicit about limit, or do some recursive fetching here. (probably just note the limits for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I don't think we gain anything from having this function abstracted out here. It's only used once and its fairly specific. Is it out here for future tests? Its just making me go back and forth between line 42 and line 175 to read it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, just not sure what's the best way of implementing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't really expecting so many changes needed in commit e1a93af. I would have been ok with the hooks requiring sorted token inputs: as long as that was clear each component could pre-sort values before passing them.
The passing of tokenA
and tokenB
through the hooks does help to simplify the components, but I worry now on seeing more of the detail that we're using the token names A and B to mean too many different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, and much cleaner than before 👍
Noting that we still have identified issue to split out as future work from this in #93 (comment)
## [0.1.7](v0.1.6...v0.1.7) (2022-07-25) ### Features * 85 update swap page logic to cosmjs usage ([#93](#93)) ([ec69ea4](ec69ea4))
🎉 This PR is included in version 0.1.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |