No meta tokens #504
No meta tokens #504
Conversation
src/common/innerPlugin.ts
Outdated
| tokens[tokenId] | ||
|
|
||
| const cleanLocation = asMaybeContractLocation(networkLocation) | ||
| if (cleanLocation == null) continue |
There was a problem hiding this comment.
if an plugin doesn't use contractAddress in EdgeMetaToken (xrp doesn't) then you won't get any metatokens. Is this okay? If not, you should probably do the inverse and check if a plugin has metaTokens defined, if so then use them. If not, then derive them.
There was a problem hiding this comment.
I have made this up to the plugins to decide. Tron and all the EVM's are calling metaTokens: makeMetaTokens(builtinTokens) in their currencyInfo's, but the Ripple plugin can do whatever it needs to make this work.
src/common/tokenHelpers.ts
Outdated
| contractAddress: asString | ||
| }) | ||
|
|
||
| export function getEosTokenId(token: EdgeToken): string { |
There was a problem hiding this comment.
This doesn't feel clean to have asset specific logic in a common file. I would prefer to move these to asset specific files. Besides, they are only used by those specific asset files.
There was a problem hiding this comment.
I deleted all of them, and just left the common cleaner.
src/ethereum/info/avaxInfo.ts
Outdated
| import { EthereumFees, EthereumSettings } from '../ethTypes' | ||
| import type { EthereumFees, EthereumSettings } from '../ethTypes' | ||
|
|
||
| const builtinTokens: EdgeTokenMap = makeTokenMap(getEvmTokenId, [ |
There was a problem hiding this comment.
Hmm. I'm not liking that the "source of truth" is in a weird array format that's neither the old or new EdgeTokenMap. Why not just use an EdgeTokenMap? That's how XRP is coded.
There was a problem hiding this comment.
Admittedly you'd want some code to automate the conversion the first time but then you're done and can commit the results. Given that the format of the EdgeMetaToken array and the new array are different, I presume you did use automation to do the conversion. Might as well go straight to a token map
There was a problem hiding this comment.
I did the conversion using regular expression search & replace. For the EdgeTokenMap conversion, I had to do it by hand. Now that it's done, we have unit tests to ensure that it always stays correct going forward.
src/ethereum/info/bscInfo.ts
Outdated
| metaTokens: [ | ||
| // Array of objects describing the supported metatokens | ||
| ] | ||
| metaTokens: [] // Deprecated |
There was a problem hiding this comment.
Wow. how do we not have any tokens on BSC?
They are mixing `require` and `export` in a bad way that breaks Sucrase.
The old `metaTokens` are deprecated.
swansontec
force-pushed
the
william/no-meta-tokens
branch
from
48a34df to
1ec71c8
Compare
| @@ -0,0 +1,31 @@ | |||
| diff --git a/node_modules/@tronscan/client/src/utils/tronWeb.js b/node_modules/@tronscan/client/src/utils/tronWeb.js | |||
There was a problem hiding this comment.
I have submitted this patch upstream at tronscan/tronscan-node-client#26
| import { TronNetworkInfo } from './tronTypes' | ||
| import type { TronNetworkInfo } from './tronTypes' | ||
|
|
||
| const builtinTokens: EdgeTokenMap = { |
There was a problem hiding this comment.
The previous code flow is a bit tough to follow, but wouldn't Tron tokens also go through the loadInnerPlugin() routine which calls upgradeMetaTokens which would have lowercased the tokenIds? This is what I ran into when implementing XRP even though my getTokenId didn't do the lowercasing. While the true upper/lower case tokenIds would have been preferred for Tron, we might have tokenIds already saved in user's accounts that are all lowercase.
There was a problem hiding this comment.
After a sidebar with Matthew to discuss pros & cons, we have decided to go ahead and change the tokenId's. We have a plan for dealing with the fallout, which isn't as terrible as we thought at first.
This removes
metaTokensfrom all our currency info's.A big goal here is to have a common source of truth for each currency's tokenId's, so there is no risk of a human or a plugin being inconsistent. We also want to be really clear about what code lives in "info land", and is therefore cheap to load, and which code lives in "currency land", and is therefore expensive to load.
The engines may still refer to these legacy tokens internally, so that still needs to be cleaned up in another PR. We do pass the builtin tokens in our
PluginEnvironment, so the engine has access to them.