Celocli beta crashes on commands where `--node` is not Mainnet

arthurgousset commented 3 months ago

Package

@celo/celocli

Have you ensured that all of these are up to date?

[X] the package you are using

What version of the package are you on?

@celo/celocli/5.0.0-beta.0 darwin-arm64 node-v18.17.1

What command or function is the bug in?

celocli network:whitelist

Operating System

macOS (Apple Silicon)

Describe the bug

Running the command with the node set to Alfajores throws an error:

$ celocli network:whitelist --node alfajores

received error while cleaning up Error: 0x765DE816845861e75A25fCA122bb6898B8B1282a is not a valid fee currency. Available currencies:
0x03d3daB843e6c03b3d271eff9178e6A96c28D25f - GoodDollar Dev (G$)
0x10c892A6EC43a53E45D0B916B4b7D383B1b78C0F - Celo Euro (cEUR)
0x1aD7b617cB6c5156A6dea6E47514D16476b99F38 - ExampleFeeCurrency (EFC) (adapted token: 0x38042de2196A07f277FC8F5E5Dfce894E0658dBB)
0x4822e58de6f5e485eF90df51C41CE01721331dC0 - USDC (USDC) (adapted token: 0x2F25deB3848C207fc8E0c34035B3Ba7fC157602B)
0x7d027790998f714B294c96Fda9E27AF586d1EbB5 - ExampleFeeCurrency (EFC) (adapted token: 0xa238271C8cd4C58c8e7e01afC4fb8120dD506977)
0x874069Fa1Eb16D44d622F2e0Ca25eeA172369bC1 - Celo Dollar (cUSD)
0xB0FA15e002516d0301884059c0aaC0F0C72b019D - ECO CFA (eXOF)
0xC4f86E9B4A588D501c1c3e25628dFd50Bc8D615e - TetherToken (USD₮)
0xDB93874fE111F5a87Acc11Ff09Ee9450Ac6509AE - Stable Test (STBLTEST) (adapted token: 0x780c1551C2Be3ea3B1f8b1E4CeDc9C3CE40da24E)
0xE4D517785D091D3c54818832dB6094bcc2744545 - Celo Brazilian Real (cREAL)
0xc9cce1e51F1393CE39EB722E3e59eDE6faBf89fD - Stable Test (STBLTEST) (adapted token: 0x780c1551C2Be3ea3B1f8b1E4CeDc9C3CE40da24E)
    at Whitelist.init (/Users/arthur/.config/yarn/global/node_modules/@celo/celocli/lib/base.js:130:23)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Whitelist._run (/Users/arthur/.config/yarn/global/node_modules/@celo/celocli/node_modules/@oclif/core/lib/command.js:303:13)
    at async Config.runCommand (/Users/arthur/.config/yarn/global/node_modules/@celo/celocli/node_modules/@oclif/core/lib/config/config.js:432:25)
    at async run (/Users/arthur/.config/yarn/global/node_modules/@celo/celocli/node_modules/@oclif/core/lib/main.js:92:16)
    at async /Users/arthur/.config/yarn/global/node_modules/@celo/celocli/bin/run.js:6:3
    Error: 0x765DE816845861e75A25fCA122bb6898B8B1282a is not a valid fee currency. Available currencies:
    0x03d3daB843e6c03b3d271eff9178e6A96c28D25f - GoodDollar Dev (G$)
    0x10c892A6EC43a53E45D0B916B4b7D383B1b78C0F - Celo Euro (cEUR)
    0x1aD7b617cB6c5156A6dea6E47514D16476b99F38 - ExampleFeeCurrency (EFC) (adapted token: 0x38042de2196A07f277FC8F5E5Dfce894E0658dBB)
    0x4822e58de6f5e485eF90df51C41CE01721331dC0 - USDC (USDC) (adapted token: 0x2F25deB3848C207fc8E0c34035B3Ba7fC157602B)
    0x7d027790998f714B294c96Fda9E27AF586d1EbB5 - ExampleFeeCurrency (EFC) (adapted token: 0xa238271C8cd4C58c8e7e01afC4fb8120dD506977)
    0x874069Fa1Eb16D44d622F2e0Ca25eeA172369bC1 - Celo Dollar (cUSD)
    0xB0FA15e002516d0301884059c0aaC0F0C72b019D - ECO CFA (eXOF)
    0xC4f86E9B4A588D501c1c3e25628dFd50Bc8D615e - TetherToken (USD₮)
    0xDB93874fE111F5a87Acc11Ff09Ee9450Ac6509AE - Stable Test (STBLTEST) (adapted token: 0x780c1551C2Be3ea3B1f8b1E4CeDc9C3CE40da24E)
    0xE4D517785D091D3c54818832dB6094bcc2744545 - Celo Brazilian Real (cREAL)
    0xc9cce1e51F1393CE39EB722E3e59eDE6faBf89fD - Stable Test (STBLTEST) (adapted token: 0x780c1551C2Be3ea3B1f8b1E4CeDc9C3CE40da24E)

The same command works on Mainnet:

$ celocli network:whitelist --node mainnet
Available currencies:
0x73F93dcc49cB8A239e2032663e9475dd5ef29A08 - ECO CFA (eXOF)
0x765DE816845861e75A25fCA122bb6898B8B1282a - Celo Dollar (cUSD)
0xD8763CBa276a3738E6DE85b4b3bF5FDed6D6cA73 - Celo Euro (cEUR)
0xe8537a3d056DA446677B9E9d6c5dB704EaAb4787 - Celo Brazilian Real (cREAL)

shazarre commented 3 months ago

most likely the reason for it is because the config is shared across networks (it's global) and because the addresses differ between networks it will fail, I see few solutions right now:

make config per network (or at least for the fee currency) - requires more effort, but allows for kinda seamless changes between networks
do not throw an error (maybe a notice only?) when a wrong address is read from and fallback to default behaviour
a variation of the previous one, but with a prompt ("you provided a gasCurrency that is not whitelisted, do you want to pay in Celo instead?")

aaronmgdr commented 3 months ago

openingly thinking of the scenarios

you might set the node and gasCurrency in the config and then

pass in the --node just for one command
pass in gasCurrency just for one command
update the default node url / network with set:config
update the feeCurrency with set:config

1 and 3 will cause problems. 2 and 4 not so much

for 3. one option is to set gasCurrency to null if none is passed. rather than getting from old config. (and best to show that info to the user if there was one ("a gas token was being used on previous network if youd like to use one on this network please set")

config per network could be compatible with this too al though the config is also what stores the network.

for situation 1. A per network config would make this a none issue. otherwise would have to either fail nicer or revert to celo

aaronmgdr commented 3 months ago

having said that my view now is config like

{
  node: url
  gasCurrency: LEGACY | {
    [chainID]: 0xString | CELO
  }
}

aaronmgdr commented 3 months ago

this is harder than i expected. mostly because i've been trying to keep the tests working but i think that is kinda fruitless. beyond the issue with changing networks i see a few other issues that have me want to change things around

1/ when calling config:set the current contractkit instance is used (so if you were on 'alfajores' and call config:set -- node 'mainnet' --gasCurrency <string for CUSD on mainnet> it would have failed)

it converts LegacyConfig. but only when calling writeConfig. if i was to run config:get only readConfig is called and so the legacy one would be shown
writeConfig will take a Legacy Config or a new Config. but i now need to merge new configs (per chain id). and since readConfig might return a Legacy i would need to do a lot of logic outside of the function around merging gasCurrency: {[chainIdAlfajores]: "0x",...previous} what im getting at is doing conversions inside write feels like its making things extra complicated
getGasCurrency will secretly do a write if it finds an old config. if reads are happening on writes at all i think it should be in readConfig.so it always happens

aaronmgdr commented 3 months ago

instead i think i will move the conversion to the readConfig.

there is already precident for it with the nodeUrl to node conversion

arthurgousset commented 3 months ago

Thanks for working on this @aaronmgdr! One idea (out-of-scope for this PR), we could consider removing the option to set a default --node and --gasCurrency in config:<...>. I'm not sure setting a default node and fee currency is particularly useful. Particularly, considering the implementation is relatively complex.

Effectively, all commands would require the --node and --gasCurrency global flags to be set explicitly in every command. That would simplify the celocli code base and provide more deterministic behaviour for users (e.g. avoid conflicts with configs that were set in the past, but forgotten).

What do you think about that @aaronmgdr ?

aaronmgdr commented 3 months ago

I think i actually have something that works well now.

only thing is there are tests for writing the config like

 test('gasCurrency StableToken (legacy)', async () => {
      await writeConfig(PATH, { gasCurrency: 'cUSD' }, kit)

im not sure why we added tests for that as i thought we decided not to support that anymore

aaronmgdr commented 3 months ago

Thanks for working on this @aaronmgdr! One idea (out-of-scope for this PR), we could consider removing the option to set a default --node and --gasCurrency in config:<...>. I'm not sure setting a default node and fee currency is particularly useful. Particularly, considering the implementation is relatively complex.

Effectively, all commands would require the --node and --gasCurrency global flags to be set explicitly in every command. That would simplify the celocli code base and provide more deterministic behaviour for users (e.g. avoid conflicts with configs that were set in the past, but forgotten).

What do you think about that @aaronmgdr ?

i dont like so much. i actually find it hard to undersand that you don't think setting a default is useful.

I think its more or less solved now and we should only be considering solutions that are IN scope.

aaronmgdr commented 3 months ago

we fixed this by removing gasCurrency from config.

celo-org / developer-tooling