Use ContractKit to get addresses for Blockchain API #1175
Conversation
|
Note that changes to .yaml files assume that all networks have a hosted node at https://{NETWORK_NAME}-infura.celo-testnet.org/ |
Codecov Report
@@ Coverage Diff @@
## master #1175 +/- ##
=======================================
Coverage 66.47% 66.47%
=======================================
Files 262 262
Lines 7648 7648
Branches 441 441
=======================================
Hits 5084 5084
Misses 2464 2464
Partials 100 100
Continue to review full report at Codecov.
|
packages/blockchain-api/package.json
Outdated
| @@ -16,6 +16,7 @@ | |||
| "deploy": "./deploy.sh" | |||
| }, | |||
| "dependencies": { | |||
| "@celo/contractkit": "0.1.5", | |||
There was a problem hiding this comment.
I believe this should use 0.1.6 if we're keeping to the scheme we decided in #eng-typescript
https://celo-org.slack.com/archives/CL6JFBALE/p1569937790004100
There was a problem hiding this comment.
Actually, sorry, looks like this isn't quite as decided as I thought it was. See continued discussion here:
https://celo-org.slack.com/archives/CL6JFBALE/p1570136686024300
| @@ -51,6 +45,8 @@ export interface BlockscoutTransaction { | |||
| } | |||
|
|
|||
| export class BlockscoutAPI extends RESTDataSource { | |||
| tokenAddressMapping: { [key: string]: string } | undefined | |||
There was a problem hiding this comment.
Any reason why we cache these values here in this class and also in the utils file?
There was a problem hiding this comment.
I cached these both places to avoid passing them back and forth. I realize this isn't the cleanest solution, but the alternatives of:
- Only caching them in the class, and passing them (sometimes undefined) in as parameters to the getter function is a weird pattern
- Only caching them in the function, and needing to call and store the addresses in the class each time they're used
Seemed less desirable
There was a problem hiding this comment.
Only caching them in the function, and needing to call and store the addresses
Maybe I'm missing something but why does the class need to store the addresses at all when they'll always be available in util's module-level cache after first retrieval? Can't we just call get every time we want them?
| let attestationsAddress: string | ||
| let tokenAddressMapping: { [key: string]: string } | ||
| export async function getContractAddresses() { | ||
| if (goldTokenAddress && stableTokenAddress && attestationsAddress) { |
There was a problem hiding this comment.
some error handling around this function contents would be good.
|
|
||
| let contractKit: ContractKit | ||
| export async function getContractKit(): Promise<ContractKit> { | ||
| if (contractKit && (await contractKit.isListening())) { |
| @@ -16,6 +16,7 @@ | |||
| "deploy": "./deploy.sh" | |||
| }, | |||
| "dependencies": { | |||
| "@celo/contractkit": "0.1.6", | |||
There was a problem hiding this comment.
@annakaz this isn't available on npm so we should use 0.1.5
There was a problem hiding this comment.
0.1.6 has now been published, so leaving this version as is
https://www.npmjs.com/package/@celo/contractkit
packages/blockchain-api/src/utils.ts
Outdated
| if (goldTokenAddress && stableTokenAddress && attestationsAddress) { | ||
| console.info('Already got token addresses') | ||
| return { tokenAddressMapping, attestationsAddress } | ||
| } else { |
There was a problem hiding this comment.
Nit: No need for this else, the if statement returns
See here:
https://stackoverflow.com/questions/268132/invert-if-statement-to-reduce-nesting
packages/blockchain-api/src/utils.ts
Outdated
| if (contractKit && (await contractKit.isListening())) { | ||
| // Already connected | ||
| return contractKit | ||
| } else { |
| @@ -131,7 +166,7 @@ export class BlockscoutAPI extends RESTDataSource { | |||
| value: new BigNumber(event.value).dividedBy(WEI_PER_GOLD).toNumber(), | |||
| address, | |||
| comment, | |||
| symbol: CONTRACT_SYMBOL_MAPPING[event.contractAddress.toLowerCase()] || 'unknown', | |||
| symbol: this.getTokenAtAddress(event.contractAddress) || 'unknown', | |||
There was a problem hiding this comment.
This one you provide a fallback value, but not the for the other uses of getTokenAtAddress - might be nice to standardize
There was a problem hiding this comment.
The fallback above isn't provided as it's for an exchange with our Exchange contract, which must be between gold and dollars.
The fallback here is for any token transfer, which could be of a new Token whose address we don't recognize
* master: (35 commits) [Wallet] Fix top of emojis cut off in the activity feed (#1243) Adding a contract to store minimum required client version (#1081) Revert "Feature #909 proxy delegatecall (#1152)" (#1241) Use ContractKit to get addresses for Blockchain API (#1175) Feature #909 proxy delegatecall (#1152) Fix Faucet done message (#1217) Updated SETUP.md with new yarn process (#1224) Adding `increaseAllowance` and `decreaseAllowance` methods (#1196) extracting function signatures (#1061) Fix integration hardcode (#1208) Fixing flaky governance test (#1155) Restore CI branch (#1223) [wallet] e2e back to green (#1210) [Wallet] Implement new import wallet flow designs (#1209) [Wallet] Fix disable conditions for butons on Enter Invite screen (#1214) [protocol] Rename infrastructureFraction to proposerFraction (#1174) [ck] Proper promise treatment to avoid UnhandledPromises (#1219) [ck] Transform StableToken parameters from fixidity format (#1218) [wallet]Store encrypted local signing key (#1188) 2019-10-03 alfajores deployment (#1200) ...
Description
Use ContractKit to get addresses for Blockchain API.
Notes that the faucet address and verification rewards address remain hardcoded, as these are not stored in the Contract Registry / available from ContractKit.
Tested
Ran tests.
Have not yet tested deploying to a dev environment
Other changes
Modified tests to not be dependent on live network contract addresses
Related issues
Backwards compatibility
Yes