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.

Sign up for GitHub

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

Jump to bottom

feat: Dex Trades per Token #876

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from
Open

feat: Dex Trades per Token #876

wants to merge 39 commits into from

Conversation

janmichek
Copy link
Collaborator

@janmichek janmichek commented Aug 6, 2024 •
edited
Loading

Description

resolves #522

Demo

image

Checklist:

  • I have read and followed the Contributing Guide
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024

Deployed to https://pr-876-aescan.stg.service.aepps.com and https://pr-876-aescan-testnet.stg.service.aepps.com

@janmichek
Copy link
Collaborator Author

Regression, waiting for aeternity/ae_mdw#1815 (comment) to be resolved

janmichek
janmichek commented Nov 21, 2024
View reviewed changes
src/components/TimeToggleButton.vue
@@ -9,10 +9,10 @@
</template>

<script setup>
import { useAppStore } from '@/stores/app'
import { useUiStore } from '@/stores/ui'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not related to the feature, renamed confusing store name. App is too general

janmichek
janmichek commented Nov 21, 2024
View reviewed changes
src/stores/dex.js
@@ -13,8 +13,7 @@ export const useDexStore = defineStore('dex', () => {
return 1
}

const { data } = await axios.get(`${DEX_BACKEND_URL}/pairs/swap-routes/${tokenId}/${AE_TOKEN_ID}`)

const { data } = await axios.get(`${DEX_BACKEND_URL}/swap-routes/${tokenId}/${AE_TOKEN_ID}`)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dex changed link just today

Liubov-crypto
Liubov-crypto suggested changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@Liubov-crypto Liubov-crypto left a comment

Choose a reason for hiding this comment

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

In general LGTM.

But I have a question regarding test tokens. For example if nobody was trading token there would be no trade activity. When I open Trades tab the page will be in endless loading:
https://pr-876-aescan-testnet.stg.service.aepps.com/tokens/ct_25e9fYcVxp8qHhiHE3ZZKv1qsRZ8knvJefcUmTchZi33WANgt7?type=trades

2024-11-25.5.36.51.mov

rem

@janmichek
Copy link
Collaborator Author

In general LGTM.

But I have a question regarding test tokens. For example if nobody was trading token there would be no trade activity. When I open Trades tab the page will be in endless loading: https://pr-876-aescan-testnet.stg.service.aepps.com/tokens/ct_25e9fYcVxp8qHhiHE3ZZKv1qsRZ8knvJefcUmTchZi33WANgt7?type=trades
2024-11-25.5.36.51.mov

rem

@Liubov-crypto good catch! Finally got it fixed.

@janmichek janmichek mentioned this pull request Dec 10, 2024
Closed
3 tasks
michele-franchi
michele-franchi approved these changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@michele-franchi michele-franchi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, just one http call seems can be optimised.

src/stores/tokenDetails.js
tokenPromise.then(() => fetchTokenPrice()),
Promise.allSettled([
fetchTotalSupply(),
tokenPromise.then(() => fetchTokenPrice()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't tokenPromise called two times?

michele-franchi
michele-franchi requested changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@michele-franchi michele-franchi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

EDIT:
I just noticed that the button "Newer" seems to always be disabled? https://pr-876-aescan.stg.service.aepps.com/tokens/ct_2U1usf3A8ZNUcZLkZe5rEoBTxk7eJvk9fcbRDNqmRiwXCHAYN?type=trades

dex-trades.mov

@janmichek
Copy link
Collaborator Author

EDIT: I just noticed that the button "Newer" seems to always be disabled? https://pr-876-aescan.stg.service.aepps.com/tokens/ct_2U1usf3A8ZNUcZLkZe5rEoBTxk7eJvk9fcbRDNqmRiwXCHAYN?type=trades
dex-trades.mov

Good catch, ser. There was FE wrong implementation + its a blocking MDW issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Liubov-crypto Liubov-crypto Liubov-crypto requested changes

@michele-franchi michele-franchi michele-franchi requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DEX trades per token
3 participants
@janmichek @michele-franchi @Liubov-crypto