-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BCI-1511: Gauntlet Test Link #355
Conversation
@@ -61,5 +61,18 @@ jobs: | |||
with: | |||
name: chainlink-cosmos | |||
authToken: '${{ secrets.CACHIX_AUTH_TOKEN }}' | |||
- name: Cache Wasmd Artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache the root dir "artifacts/" folder which contains the optimized .wasm bytecode files. This usually takes a while to compile
@@ -24,6 +24,8 @@ integration-tests/logs | |||
integration-tests/smoke/logs | |||
integration-tests/soak/logs | |||
tmp-manifest-* | |||
artifacts | |||
/artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only cache the root directory artifacts folder (we commit to source other artifacts/ folders for cw-plus contracts)
.local-mock-server | ||
codegen | ||
devAccounts.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json file that contains test accounts for local wasmd -- is created everytime local wasmd is setup in the script (see other code below)
.local-mock-server | ||
codegen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
folder that is auto-generated by https://github.com/CosmWasm/ts-codegen
@@ -22,6 +22,7 @@ backtraces = ["cosmwasm-std/backtraces"] | |||
library = [] | |||
|
|||
[dependencies] | |||
cosmwasm-schema = { version = "1.1.5" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to move cosmwasm-schema from dev-dependency to dependency to use the new write_api! macro
export_schema(&schema_for!(InstantiateMsg), &out_dir); | ||
export_schema(&schema_for!(ExecuteMsg), &out_dir); | ||
export_schema(&schema_for!(QueryMsg), &out_dir); | ||
write_api! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of individually, writing the schema for instantiate, execute, and query messages, the write_api! macro generates all of them + a centralized json file that contains all the json fields necessary to describe the entire contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move contracts/rust/non-gauntlet changes into a separate PR? 🙏
|
||
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] | ||
#[cw_serde] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cw_serde macro includes all of the commonly used macros besides Eq
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] | ||
#[serde(rename_all = "snake_case")] | ||
#[cw_serde] | ||
#[derive(Eq, QueryResponses)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QueryResponses allows you to associate the return type with the query message. This is a must have for ts-codegen
let mut other_dir = out_dir.clone(); | ||
other_dir.push("other"); | ||
create_dir_all(&other_dir).unwrap(); | ||
export_schema(&schema_for!(State), &other_dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we actually use this State schema but I kept it here in case we do. I'll remove it if I find it's unused after all gauntlet tests are finished
@@ -23,5 +23,16 @@ module.exports = { | |||
}, | |||
}, | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add gauntlet-cosmos-cw-plus to be discoverable as a jest project
@@ -1,4 +1,4 @@ | |||
NODE_URL=http://localhost:26657 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason, wasmd does not support ipv6 so you have to specify the ipv4 address
@@ -58,7 +58,9 @@ export abstract class Contract { | |||
if (version === 'local') { | |||
// Possible paths depending on how/where gauntlet is being executed | |||
const possibleContractPaths = [ | |||
path.join(__dirname, '../../../../artifacts'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make artifacts discoverable using __dirname
@@ -95,19 +101,14 @@ export abstract class Contract { | |||
const abi = possibleContractPaths | |||
.filter((path) => existsSync(`${path}/${this.dirName}/schema`)) | |||
.map((contractPath) => { | |||
const toPath = (type) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simply contract path logic
@@ -0,0 +1,61 @@ | |||
const codegen = require('@cosmwasm/ts-codegen').default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codegen reads the json api from each path to generate the typed client
@@ -4,7 +4,7 @@ container_name="chainlink-cosmos.wasmd" | |||
container_version="v0.40.1" | |||
genesis_account="wasm1lsagfzrm4gz28he4wunt63sts5xzmczwda8vl6" | |||
|
|||
set -euo pipefail | |||
set -euox pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log output
scripts/wasmd.sh
Outdated
@@ -35,7 +35,8 @@ docker run \ | |||
--name "${container_name}" \ | |||
"cosmwasm/wasmd:${container_version}" \ | |||
"./setup_and_run.sh" \ | |||
"${genesis_account}" | |||
"${genesis_account}" \ | |||
"$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass in more account addresses to be created
@@ -13,7 +13,7 @@ stdenv.mkDerivation rec { | |||
sha256 = "sha256-KBchSnIETM+HPngqHeajiqLYrhHhXY531C+Q/aSDzl4="; | |||
}; | |||
|
|||
buildInputs = with pkgs; [ gnumake git go_1_20 which openssl cacert ]; | |||
buildInputs = with pkgs; [ gnumake git go_1_20 which openssl cacert gcc ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix wasmd build on mac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, added some initial comments!
cache-name: cache-wasmd-artifacts | ||
with: | ||
path: artifacts | ||
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ hashFiles('contracts') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great optimization, the wasmd artifacts build step is also pretty slow in some other workflows. i'm not sure this key
is sufficient though - what if the Makefile or packages-ts
changes? couldn't that affect artifacts output?
for example, some .wasm
artifacts are directly downloaded in packages-ts
:
readonly downloadUrl = `https://github.com/CosmWasm/cw-plus/releases/download/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just for the chainlink contracts that we write (since it is just calling build-contracts.sh at the end of the day). the cosmos base contracts will always be downloaded I believe the way things are written now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please double check, but i believe the caching step occurs at the end of your workflow - which means it would store everything that's been placed in the artifacts/
path, and then restore everything, including downloaded wasm code. there is then a file exists check here which would stop a download of a new wasm file from happening:
.filter((contractPath) => existsSync(`${contractPath}/${this.id}.wasm`)) |
ideally, the caching should only cache the compiled contracts. perhaps it can be moved into a subdirectory/isolated from artifacts/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what my root artifacts/ folder looks like after running local tests ~
I'm pretty confident that the cw-plus contracts are never stored in the artifacts/ folder in the default workflow because it is read as an array buffer.
The way it's written, I think the original author intended for cw-plus contracts to be downloaded (
constructor(id, dirName, defaultVersion = DEFAULT_CWPLUS_VERSION) { |
const otherAddresses = accounts.map((a) => a.address).join(' ') | ||
const wasmdScript = path.join(__dirname, '../../../scripts/wasmd.sh') | ||
|
||
execSync(`${wasmdScript} ${otherAddresses}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm curious if we should be adding other addresses as validators. if these are meant to be normal addresses that only require some tokens (link/native) - could we just send it to them from the deployer address instead?
we actually do this in the integration test:
for i, nodeAddr := range chainlinkClient.GetNodeAddresses() { |
but reconsidering that implementation - this would be nice to have as a gauntlet action to use here and in these tests as well
cc @calvwang9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point, I think the original validator already has enough token as it is so I'll just create new accounts within the test
export_schema(&schema_for!(InstantiateMsg), &out_dir); | ||
export_schema(&schema_for!(ExecuteMsg), &out_dir); | ||
export_schema(&schema_for!(QueryMsg), &out_dir); | ||
write_api! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move contracts/rust/non-gauntlet changes into a separate PR? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! just small nits from me
const MNEMONIC = | ||
'surround miss nominee dream gap cross assault thank captain prosper drop duty group candy wealth weather scale put' | ||
const NODE_URL = 'http://127.0.0.1:26657' | ||
const DEFAULT_GAS_PRICE = '0.025ucosm' | ||
const NETWORK = 'local' | ||
const TIMEOUT = 90000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not all of these consts are used and some are already defined in utils.ts
* | ||
* @returns {string[]} Initialized account addresses | ||
*/ | ||
export const maybeInitWasmd = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 'maybeInitWasmd' seems kinda confusing semantically (is wasmd initialized or not?)- perhaps just 'initWasmd' would suffice?
UPDATE: This PR has been enveloped into #364
Most of the line code changes are from modifying the generated json schemas
Please filter out .json files to better review the PR
Couple of changes for this