Skip to content

Comments

Feature/chains improvement#1955

Open
n13 wants to merge 6 commits intomasterfrom
feature/chains_improvement
Open

Feature/chains improvement#1955
n13 wants to merge 6 commits intomasterfrom
feature/chains_improvement

Conversation

@n13
Copy link
Member

@n13 n13 commented Jun 12, 2024

No description provided.


const seedsToken = TokenModel(
chainName: "Telos",
chainName: SeedsChains.telos.value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems we can't use extensions in constant expressions. Ref explanation here: dart-lang/language#663

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe you need to update your flutter version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you're right - there's something weird going on with this PR.

@@ -0,0 +1,26 @@
const String _chainTelos = 'telos';
Copy link
Collaborator

@chuck-h chuck-h Jun 30, 2024

Choose a reason for hiding this comment

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

Lower case "telos" makes sense but we have a conflict with legacy code in seeds.tokensmaster.cpp and the existing tmastr.seeds table entries which use "Telos".

Suggested fix would be to patch a conditional expression here which specifically converts "Telos" to "telos".

chainName: parsedJson["chain"]!,

e.g.

     chainName: parsedJson["chain"]! == "Telos" ? "telos" : parsedJson["chain"]!, // fix legacy capitalization

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this an actual bug or ... ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to choose whether "telos" or "Telos" is the string we use in TokenModel.chainName.

  1. If "Telos" (capitalized), change
  2. If "telos" (lowercase), make the patch so that legacy metadata containing "Telos" is normalized.

So yes, it's a bug that will appear as soon as we start using this PR code.

@chuck-h
Copy link
Collaborator

chuck-h commented Jun 30, 2024

@n13 This looks like the beginning of an evolution to multichain capability as we will be able to show token cards from Telos and EOS (and other Antelope chains as well). IMHO this is great and I love it!. Is there a bigger plan for how this UX should work?

update
I suggest that the wallet state at any given time includes the "active chain". When the active chain is telos, behavior is pretty much what we have now, including SEEDS-specific functions. With a different active chain, the SEEDS stuff is disabled.

Currency cards are filtered by the active chain. Active chain is set when you switch to a new account; the account selector tab should have a UI indication of chain.

Token metadata for all Antelope chains can be managed thru the tmastr.seeds contract on Telos.

Is it true that only Telos retains the original eosio resource payment model? In any case, SEEDS payforcpu doesn't make sense for non-SEEDS accounts on other chains, and we may have to figure out how to integrate PowerUp etc.

@n13
Copy link
Member Author

n13 commented Jul 3, 2024

Merged master into this - probably need to do a new PR as it is it makes little sense

},
).catchError((dynamic error) => _statRepository.mapHttpError(error));
).catchError((dynamic error) {
// This entire code here is really funky - Nik
Copy link
Collaborator

@chuck-h chuck-h Jul 3, 2024

Choose a reason for hiding this comment

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

I resemble this remark! ;)

see #1960

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.

2 participants