Gauntlet sec improvements #166

RodrigoAD · 2022-03-02T16:47:19Z

Added user confirmation with RDD checks in ownership commands
Offchain config can be inspected without digest. Will get the offchain config from the tx log

sdrug

Lots of overlapping with what I have regarding fetching configs in beforeExecute, I will have to sync with this

krebernisak · 2022-03-03T10:47:33Z

packages-ts/gauntlet-terra-contracts/src/commands/contracts/ownership/acceptOwnership.ts

+    logger.info(`Accepting Ownership Transfer of contract with current owner ${currentOwner} to new owner `)
+    await prompt('Continue?')
+    return
+  }
+  const contract = getContractFromRDD(getRDD(context.flags.rdd), context.contract)
+  logger.info(`Accepting Ownership Transfer of contract of type "${contract.type}":
+    - Contract: ${contract.address} ${contract.description ? '- ' + contract.description : ''}
+    - Current Owner: ${currentOwner}
+    - Next Owner (Current signer): ${signer}


Seems like only the contract description is fetched from RDD, so consider having one log line with most data in all cases, then in case RDD is present just print extra contract.description.

Also the contract type (proxy, contract, ac), and basically verifies that the contract actually exists on RDD

krebernisak · 2022-03-03T10:48:35Z

packages-ts/gauntlet-terra-contracts/src/commands/contracts/ownership/transferOwnership.ts

+    logger.warn('No RDD flag provided. Transferring without RDD check')
+    logger.info(
+      `Proposing Ownership Transfer of contract with current owner ${currentOwner} to new owner ${context.contractInput.to}`,
+    )
+    await prompt('Continue?')
+    return
+  }
+  const contract = getContractFromRDD(getRDD(context.flags.rdd), context.contract)
+  logger.info(`Proposing Ownership Transfer of contract of type "${contract.type}":
+    - Contract: ${contract.address} ${contract.description ? '- ' + contract.description : ''}
+    - Current Owner: ${currentOwner}
+    - Next Owner: ${context.contractInput.to}
+  `)


Same comment as above. There is an opportunity to clean up these log outputs.

packages-ts/gauntlet-terra-contracts/src/lib/provider.ts

krebernisak · 2022-03-03T10:54:03Z

packages-ts/gauntlet-terra/src/commands/middlewares.ts

@@ -37,7 +37,7 @@ export const withWallet: Middleware = async (c: TerraCommand, next: Next) => {
  }

  c.wallet = c.provider.wallet(key)
-  logger.info(`Operator address is ${c.wallet.key.accAddress}`)
+  logger.debug(`Operator address is ${c.wallet.key.accAddress}`)


This seems more relevant than debug? 🤔

Multisig is currently wrapping 3 commands, and this can become noisy, as will be prompted 3 times in that case. I guess we could move the log somewhere else

krebernisak · 2022-03-03T10:58:54Z

packages-ts/gauntlet-terra-contracts/src/lib/rdd.ts

+export type RDDContract = {
+  type: CONTRACT_TYPES
+  contract: any
+  address: string
+  description?: string
+}
+
+export const getContractFromRDD = (rdd: any, address: string): RDDContract => {
+  return Object.values(CONTRACT_TYPES).reduce((agg, type) => {
+    const content = rdd[type]?.[address]
+    if (content) {
+      return {
+        type,
+        contract: content,
+        address,
+        ...((type === CONTRACT_TYPES.CONTRACT || type === CONTRACT_TYPES.PROXY) && { description: content.name }),
+      }
+    }
+    return agg
+  }, {} as RDDContract)
+}


This should also be moved to gauntlet-terra as a first step, probably to core afterward. For example, we could make the CONTRACT_TYPES an input parm to getContractFromRDD fn as a filter, but probably this fn needs some more thought.

If not quick, feel free to just add a comment explaining the desired next steps.

Not sure about moving it to gauntlet-terra, is quite contract specific no?

I imagine this function to change in the future, it can become quite useful. We can update it when we have other use cases for it

That's why it needs more thought, and feel free to leave it here s is.

But IMO high-level ideas like RDDContract & getContractFromRDD (not talking about this specific impl) is a core thing and needs to be moved to the core level.

Moved to gauntlet-terra, and agree, this should go into a common place. Opening an issue on it

krebernisak · 2022-03-03T11:05:34Z

packages-ts/gauntlet-terra-contracts/src/commands/contracts/ocr2/inspection/inspect.ts

  }
 }

 const makeInspectionData = () => async (input: ContractExpectedInfo): Promise<ContractExpectedInfo> => input

-const makeOnchainData = (query: Query) => async (
+const makeOnchainData = (query: Query, search: Search) => async (


Anyone reading this first question will be what's the difference between a Query and a Search 🤔

Why don't we have a provider as an input param and then do:

provider.wasm.contractQuery({...})

provider.tx.search({...})

It would make the code a lot more understandable IMO.

Yeah I've thought on that too, this started with the query function only, but I can see it growing.
Let me test if we can include the provider

With provider works like a charm

packages-ts/gauntlet-terra-contracts/src/commands/abstract/executionWrapper.ts

krebernisak · 2022-03-03T11:17:09Z

packages-ts/gauntlet-terra-contracts/src/commands/abstract/executionWrapper.ts

@@ -21,7 +28,7 @@ export interface AbstractInstruction<Input, ContractInput> {
  makeInput: (flags: any, args: string[]) => Promise<Input>
  validateInput: (input: Input) => boolean
  makeContractInput: (input: Input) => Promise<ContractInput>
-  beforeExecute?: (context: BeforeExecutionContext<Input, ContractInput>) => () => Promise<void>
+  beforeExecute?: BeforeExecute<Input, ContractInput>
  afterExecute?: (response: Result<TransactionResponse>) => any


Feel free to also type this function, something like:

export type AfterExecute<Input, ContractInput> = ( context: ExecutionContext<Input, ContractInput>, response: Result<TransactionResponse>, ) => Promise<any>

Additionally, not too fond of these <Input, ContractInput> types overall but no suggestions yet.

krebernisak · 2022-03-03T11:18:28Z

packages-ts/gauntlet-terra-contracts/src/commands/abstract/executionWrapper.ts

 }

+export type BeforeExecute<Input, ContractInput> = (


Why is this a function, returning a function, returning a promise?

Makes more sense for the BeforeExecute type to be a simple function returning a promise?

The instruction command will provide the context in what the execution will happen. Then is responsibility of the command executor to actually use the hook.
IE, multisig command don't have the context of what's happening in the underlying commands, just wants to trigger the hook

… so the inspect command can check all values have changed No longer use the digest in the test as it is not needed

* added balance diff in inspection * transfer token and send uluna commands * added mainnet ids * refactor before execution * refactor and improvements * fix rebase * Gauntlet sec improvements (#166) * transfer ownership checks * inpect offchain config from event info * provider in execution context. minor improvements * updated test * hex to base64 * Update guantlet e2e test to use an rdd with all non zero false values so the inspect command can check all values have changed No longer use the digest in the test as it is not needed Co-authored-by: Tate <[email protected]> * added cw20 code id and more validations * small refactor Co-authored-by: Tate <[email protected]>

* bugfix: fix abstract command abi validation to work with cw-plus contracts These use anyOf instead of oneOf to list the available functions * add report output (#131) Also adds the option of setting what the report name will be via the environment variable "REPORT_NAME" which aids in automated tests. * Remove crates/query-proxy We no longer have the flags proxy so there's no sense to share definitions. * Multisig wrapper command (#111) * commands export data raw tx on inspection command * basic package and schema * multisig command * multisig command improvements list some todos * more detailed state * execute option on multisig * refactor * Feature/29149 multisig group commands (#132) * 29141: update_admin command * 29149: update_members command * 29149: export update_admin and update_memners commands * 29149: MR feedback - file reorganisation * 29149: applying formatting * pkg/terra: add Test_parseAttributes (#128) * fix tests (#139) * correctly set cosmos queries * rename juelsperluna to juelsperfeecoin in the set_config event * add check to make sure all fields are extracted from logs * switch to cropping the config digest before publishing to kafka. * add test for decoding configuration, transmission and balance from the chain read * revert to using the decoded config digest * use the relay monitoring Logger interface instead of the core logger * fix terra monitoring main * migration up test boiler (#138) * fix chaos CI (#140) * refactor ocr2:deploy --id to args[0] (#137) * changed flag on proxy command (#144) * fix proxy query for decimals, version, description * Improvements after Multisig testing (#145) * inspection improvements improvements on inspection * inspect msig comand * max voting period 7 days * Document lack of transmitters prefix (#150) * additional unit tests for remaining proxy functions (#149) * add ocr2 spec changes, switch image repo (#152) * Add gauntlet e2e tests through accepting a proposal (#141) * Add a hello-world consumer example (#143) * Add a hello-world consumer example * wrap up consumer contract + test cases * remove unneeded boilerplate - simplify mocked contract Co-authored-by: aalu1418 <[email protected]> * change env naming convention (#155) * proxy cmds uses args[0] and pull aggregator from RDD (#148) * add codeId for proxy_ocr2 testnet (#undefined) * feat: update OCR2 spec examples (#154) * Add Ocr2 Proxy E2E Test (#162) * implement the contracts for proxy so we can deploy them * Add ocr2 proxy test for latestRound, decimals, and description * pkg/terra: add MsgEnqueuer.GetMsgs (#113) * fix inspection bugs + improvements (#161) * proxy monitoring stub * move chain reader into its own file * proxy source factory uses the proxy address. Also update corresponding data generator for tests * add prometheus exporter factory and test, along with metrics and mocks * fix the issues reported by the linter * bump chainlink-relay to get the panic recover feature from Source * bump chainlink-relay dependency * add support for monitoring feeds that don't have a proxy contract configured * fix url path for fetching transactions from FCD * bump to the latest relay version where tx failed/succeeded metrics are counters instead of gauges * sequence requests to the terra rpc endpoint for all sources * bump chainlink-relay * introduce individual sequencers for TxsEvents and ContractStore * bump chainlink-relay and add GetType to all the terra monitoring source * fix go.sum * Token commands + Improvements (#157) * added balance diff in inspection * transfer token and send uluna commands * added mainnet ids * refactor before execution * refactor and improvements * fix rebase * Gauntlet sec improvements (#166) * transfer ownership checks * inpect offchain config from event info * provider in execution context. minor improvements * updated test * hex to base64 * Update guantlet e2e test to use an rdd with all non zero false values so the inspect command can check all values have changed No longer use the digest in the test as it is not needed Co-authored-by: Tate <[email protected]> * added cw20 code id and more validations * small refactor Co-authored-by: Tate <[email protected]> * unit test: reverted payments do not change owed balance (#169) * Payout oracles when set_billing is called (#168) * fix: rename accept_proposal method attribute * payout oracles when billing is set * Support negative observations (#165) * Use generic helper * Add a test * Placate linter * Address comments * Linter * 30252 tx simulation (#159) * 30252: TX Simulation method in AbstractCommand * 30252: Adding isSimulate instruction field to all oc2 initialize flow commands * 30252: Formatting * 30252: Removing optional isSimulate field (i.e always simulate) * 30252: Tx simulation for the arbitrary provided signer * 30252: batch simulation * 30252: tx simulation for multisig * 30252: Adjusting simulation success message * 30252: Formatting * Fee coin bounds check (#172) * Fee coin bounds check * Sanity check reimbursement calc * Cargo fmt * include chain_id length in config digest (#173) * include chain_id length in config digest * validate length below uint8 max, test case assert error * Providing raw proposal id for command input (#179) * Multisig simulation: generating tx using provider instead of wallet to avoid default assignments (#178) * drop the namespace from the logger * make the proxy source fetch and export the link available for payments of an aggregator contract * remove the link_available_for_payments metric on cleaup * ensure Client methods respect context cancel or expired * bump chainlink-relay to get the fix on the balances/observations precision * More mocked sources for tests (#156) * More mocked sources tests * in progress * Fix regex (#182) * Feature/31183 settings diff (#171) * added balance diff in inspection * transfer token and send uluna commands * added mainnet ids * refactor before execution * refactor and improvements * transfer ownership checks * inpect offchain config from event info * added balance diff in inspection * transfer token and send uluna commands * added mainnet ids * refactor before execution * refactor and improvements * fix rebase * transfer ownership checks * inpect offchain config from event info * provider in execution context. minor improvements * 31183: adding printDiff * 31183: printDiff function * 31183: getLatestOCRConfig function * 31183: proposeConfig * 31183: proposeOffchainConfig * 31183: acceptProposal * 31183: Small fixes based on the code review * 31183: randomSecret requested while creating the proposal to guarantee deterministic check * 31183: Cleanup * 31183: Resolving conflicts with token-commands * added cw20 code id and more validations * 31183: longsToNumber into a separate function * 31183: Updating inspect command with getLatestOCRConfig * refactor encryptions to accept secret * 31183: Adding payees to proposedConfig * Update e2e gauntlet test to grab the offchain proposal secret from the report and use it in the accept proposal * lint format * 31183: make longsInObjToNumbers pure and add deepCopy implementation to utils * 31183: Formatting * 31183: Review implementation - reorderings, renamings, comments Co-authored-by: RodrigoAD <[email protected]> Co-authored-by: Tate <[email protected]> * Add instructions for running the e2e tests to the readme (#174) * Add instructions for running the e2e tests to the readme * Move to docs and use readme link to it * bring solana and terra more in sync * Defaulting migrationContract to sender accAddress (#183) * add prod testnet environment (#184) * Add default rdd setting, and require for acceptOwnership Minor changes related to -rdd gauntlet flag (suggested during multisig review): 1. Will default to "../reference-data-directory/directory-terra-mainnet.json" if not passed explicitly 2. acceptOwnership & transferOwnership will throw Error instead of warn, if there is no rdd flag The first one should make it so the second one can never happen, but this is better behavior just in case. And it matches what most other commands already do. * Update default multisig-proposal expiration time to 24 hours * Expose payees on the set_config event * Add a note regarding validation execution * Fix event names in owned crate * Fix CI by removing a non-working 'defaultFlags.rdd' * copy all versions of cosmwasm to /lib * upgrade contract codeIds to v0.1.5 * update testnet-internal and add new lines to all codeIds files Co-authored-by: Domino Valdano <[email protected]> Co-authored-by: Tate <[email protected]> Co-authored-by: Blaž Hrastnik <[email protected]> Co-authored-by: Rodrigo Ariza <[email protected]> Co-authored-by: Sergei Drugalev <[email protected]> Co-authored-by: Jordan Krage <[email protected]> Co-authored-by: Sergey Kudasov <[email protected]> Co-authored-by: alexandru topliceanu <[email protected]> Co-authored-by: Frank Zhu <[email protected]> Co-authored-by: aalu1418 <[email protected]> Co-authored-by: Connor Stein <[email protected]> Co-authored-by: Aaron Lu <[email protected]> Co-authored-by: Rens Rooimans <[email protected]> Co-authored-by: Gheorghe Strimtu <[email protected]> Co-authored-by: Akhil Chainani <[email protected]>

RodrigoAD force-pushed the token-commands branch from b946cd4 to 0bea524 Compare March 2, 2022 16:47

RodrigoAD force-pushed the gauntlet-sec-improvements branch from 189713d to b124dfa Compare March 2, 2022 16:48

RodrigoAD marked this pull request as ready for review March 3, 2022 09:50

RodrigoAD requested review from krebernisak and sdrug March 3, 2022 09:50

sdrug approved these changes Mar 3, 2022

View reviewed changes

krebernisak reviewed Mar 3, 2022

View reviewed changes

krebernisak approved these changes Mar 3, 2022

View reviewed changes

RodrigoAD force-pushed the token-commands branch from 0bea524 to 78906d7 Compare March 3, 2022 14:06

RodrigoAD force-pushed the gauntlet-sec-improvements branch from 41e60b7 to 0c3752f Compare March 3, 2022 14:07

RodrigoAD force-pushed the token-commands branch from 78906d7 to b52badf Compare March 3, 2022 14:48

RodrigoAD added 3 commits March 3, 2022 15:58

transfer ownership checks

1404c25

inpect offchain config from event info

c82a472

provider in execution context. minor improvements

9929c89

RodrigoAD force-pushed the gauntlet-sec-improvements branch from 0c3752f to 9929c89 Compare March 3, 2022 14:59

RodrigoAD and others added 3 commits March 3, 2022 16:25

updated test

92d09d1

hex to base64

e61d880

Update guantlet e2e test to use an rdd with all non zero false values…

c6b4d3e

… so the inspect command can check all values have changed No longer use the digest in the test as it is not needed

RodrigoAD merged commit c39d94f into token-commands Mar 4, 2022

RodrigoAD deleted the gauntlet-sec-improvements branch March 4, 2022 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gauntlet sec improvements #166

Gauntlet sec improvements #166

RodrigoAD commented Mar 2, 2022 •

edited

Loading

sdrug left a comment

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

krebernisak Mar 3, 2022

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

krebernisak Mar 3, 2022

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

RodrigoAD Mar 3, 2022

krebernisak Mar 3, 2022

krebernisak Mar 3, 2022

RodrigoAD Mar 3, 2022

Gauntlet sec improvements #166

Gauntlet sec improvements #166

Conversation

RodrigoAD commented Mar 2, 2022 • edited Loading

sdrug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RodrigoAD commented Mar 2, 2022 •

edited

Loading