Initial configuration verifier implementation #322
Conversation
* Checks decimals of manager on Solana and EVM * Checks the peers to ensure they line up as expected * Quick sanity check on outbound/inbound rate limiting * Lock is only a single chain and everything else is a burn * Threshold maxxed out
| } | ||
|
|
||
| // Check if the rate limit is turned off or maxed out | ||
| if(target['outboundRateLimit'].eq(0) || target['outboundRateLimit'].eq(BigNumber.from(2).pow(256).sub(1))){ |
There was a problem hiding this comment.
I think you can bring the RateLimit checks down by quite a lot:
-
Solana uses u64 to store this value so this check will never trigger even if capacity is at its maximum value
https://github.com/wormhole-foundation/example-native-token-transfers/blob/0d764bf7ffbbf81b4d51d1bfd5e51929e7fb0460/solana/programs/example-native-token-transfers/src/queue/rate_limit.rs#L8 -
I think it's safe to say that both of these cases would count as "disabled". If the rate limit is set to be the maximum possible integer value, there is no rate limit
-
I wonder if we would add another check that's in the range of 2^32 or even smaller? To me that would still be very high for e.g. a 24h period
There was a problem hiding this comment.
Yeah! Good ideas. I'm unsure if the rateLimit has values that are trimmed or not. I'll confirm this.
The 'delay' check is good! I'll add that in.
| - Manager peers match | ||
| - Inbound rate limit for each chain is turned on | ||
| - Outbound rate limit is turned on | ||
| - Rate limit duration queue is greater than 24 hours |
There was a problem hiding this comment.
I think this should read 'greater than or equal to' given that we're using 86400 which is exactly 24 hours.
| // console.log(await manager.program.account.registeredTransceiver.fetch(accounts[0].pubkey)); | ||
| const managerConfig = await manager.getConfig(); | ||
|
|
||
| managerData['mode'] = (managerConfig['mode'].locking == undefined ? 1 : 0); // Convert to an integer for later use |
There was a problem hiding this comment.
So if .locking is undefined this should be equal to 1? Is this used more like a boolean/enum for the mode of a contract or does this represent a count similar to your burn and lock variables below?
I think the comment here could be amended to mention what the 'later use' is
| managerData['token'] = managerConfig['tokenProgram'] | ||
| managerData['threshold'] = managerConfig['threshold'].valueOf() // The 'threshold' value | ||
| managerData['count'] = managerConfig.enabledTransceivers; // Amount of active transceivers | ||
| managerData['chainId'] = 1; |
There was a problem hiding this comment.
It would be helpful to add a note here that this is the "wormhole chain ID" for Solana.
| - Consistency level is hardcoded on the Solana side to be 'finalized'. So, just hardcoding that here | ||
| */ | ||
| managerData['transceivers'] = [{ | ||
| address: transceiver_address_buffer, isWormhole: true, consistencyLevel: 1, nttManager: managerAddress |
There was a problem hiding this comment.
So consistencyLevel: 1 means finalized?
|
|
||
| break; | ||
| } | ||
| catch{} // defaults to not adding it |
There was a problem hiding this comment.
Any chance you could log something here? Having a silent try-catch might hide some important errors
| console.log(`ERROR: Transceiver inconsistent NTTManager Ownership: ${target['transceivers'][0]['address']}-${target_id}`); | ||
| } | ||
|
|
||
| continue; |
There was a problem hiding this comment.
Could you add a note here as to why the 'chain specific code' below is being skipped in this case?
| continue; | ||
| } | ||
|
|
||
| // Don't feel this is necessary or proper. Depends on the use case. |
There was a problem hiding this comment.
IMO this is a nice check, especially since you flag it as a warning rather than an error.
| } | ||
|
|
||
| // Checking if the mint/burn setup is correct | ||
| if(Object.keys(chainData).length -1 != burn || lock != 1){ |
There was a problem hiding this comment.
Isn't it possible to do only a mint-and-burn model without a locking chain? I thought this was part of the architecture at least at a certain point of the design discussions.
It might be good to add a check for this. If you don't want to complicate this PR then maybe we could add it as an Issue
| targetPeerAddress = BigNumber.from(0x000000000000000000000000000000000000000000000); // Can't find the addresss, since it doesn't exist | ||
| } | ||
| }else{ | ||
| console.log("Invaild chain type in checkTransceivers"); |
There was a problem hiding this comment.
| console.log("Invaild chain type in checkTransceivers"); | |
| console.log("Invalid chain type in checkTransceivers"); |
| 'finalized', | ||
| ); | ||
|
|
||
| managerData['token'] = "0x" + bufferToHex(managerConfig['mint'].toBuffer()); |
There was a problem hiding this comment.
I think it's worth a comment here that the 0x prefix is being added for convenience to allow comparing Solana/Eth with the same functions.
It looks a little strange to see Solana addresses with a 0x in front since these aren't used in the ecosystem (and in fact program addresses are not hex-encoded but use base58).
The goal with this code is to find bugs in deployed NTT tokens. Right now, it has the following capabilities:
Going forward, I want to make this more portable within a Docker container.