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

web-wallet: Scaffold Token pages #3442

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nortonandreev
Copy link
Contributor

@nortonandreev nortonandreev commented Jan 29, 2025

Resolves #3415

Visuals:

Screenshot 2025-02-05 at 10 53 44 Screenshot 2025-02-05 at 10 53 33

Note: I have tweaked the current Transaction type, rather than introducing a new one for the Token's Transaction. I think both approaches have flaws – one avoids overcomplicating the code, the another one introduces potentially unneeded changes to code that is currently on production. I spoke with Thomas yesterday, seems like the Token's Transaction type will be different than the Transactions – however, I still think we should still try to make the types compatible with one another, as we are in a desperate need of simplification of the Explorer.

@nortonandreev nortonandreev force-pushed the feature-3415 branch 2 times, most recently from c37e616 to 167f405 Compare February 5, 2025 08:55
@nortonandreev nortonandreev self-assigned this Feb 5, 2025
@nortonandreev nortonandreev force-pushed the feature-3415 branch 3 times, most recently from 1df7467 to 2e643e2 Compare February 5, 2025 09:06
@nortonandreev nortonandreev marked this pull request as ready for review February 5, 2025 09:12
@@ -41,6 +41,11 @@
},
];

$: if (import.meta.env.VITE_FEATURE_TOKENS === "true") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't need to be reactive; you can just:

const navigation [ /* ... */ ];

if (import.meta.env.VITE_FEATURE_TOKENS === "true") {
  navigation.push({ link: "/tokens", title: "Tokens" });
}

@ascartabelli
Copy link
Contributor

Note: I have tweaked the current Transaction type, rather than introducing a new one for the Token's Transaction. I think both approaches have flaws – one avoids overcomplicating the code, the another one introduces potentially unneeded changes to code that is currently on production. I spoke with Thomas yesterday, seems like the Token's Transaction type will be different than the Transactions – however, I still think we should still try to make the types compatible with one another, as we are in a desperate need of simplification of the Explorer.

A couple of questions here:

  • are these token transactions a possible transaction in a block?
  • can't we just have a specific type that extends the transaction one?

@nortonandreev
Copy link
Contributor Author

nortonandreev commented Feb 5, 2025

A couple of questions here:

  • are these token transactions a possible transaction in a block?

I think so.

  • can't we just have a specific type that extends the transaction one?

Yeah, this is also possible. But I didn't want to fiddle too much with the types, until we have some API spec. Otherwise it's just back and forth changing stuff on our end.

I know that @Neotamandua still haven't started working on it yet, but would be good to have some drafts / plan on how it would look like.

@ascartabelli
Copy link
Contributor

Fair enough.

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.

explorer: Build token page
2 participants