-
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-1469: Update CosmWasm contracts #370
Conversation
integration-tests/common/common.go
Outdated
@@ -127,6 +127,8 @@ func NewCommon(t *testing.T) *Common { | |||
chainlinkConfig := fmt.Sprintf(`[[Cosmos]] | |||
Enabled = true | |||
ChainID = '%s' | |||
Bech32Prefix = 'wasm' |
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.
good catch!
@@ -95,6 +95,15 @@ export abstract class Contract { | |||
const abi = possibleContractPaths | |||
.filter((path) => existsSync(`${path}/${this.dirName}/schema`)) | |||
.map((contractPath) => { | |||
if (existsSync(path.join(contractPath, `./${this.dirName}/schema/${this.dirName.replace('_', '-')}`))) { |
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 might be outside the scope of this PR and maybe a question for @augustbleeds, but i'm curious why in the repo, we have: contracts/ocr2/schema/main/ocr2.json
with the main
subfolder - but then contracts/proxy-ocr2/schema/proxy-ocr2.json
- which fits into this pattern being added.
also, with this new clause, it seems like we don't need the duplicated execute/query/instantiate files. could those be excluded from the repo or build?
eg contracts/ocr2/schema/raw/query.json
.. at the same time, there exists contracts/flags/schema/raw/query.json
but no equivalent contracts/flags/schema/flags.json
- could we be consistent here?
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.
why in the repo, we have: contracts/ocr2/schema/main/ocr2.json with the main subfolder - but then contracts/proxy-ocr2/schema/proxy-ocr2.json - which fits into this pattern being added.
For some reason, codegen did not work correctly if there was any other files besides the full abi .json and the raw/ folder OR just the full abi .json. I'm assuming this is because it expects the updated write_api! macro to be used when generating the abis. So weird behavior happens when you include additional abis on top of it. So you'll notice here that the deviationflaggingvalidator and ocr2 contracts have this special path.
Yes, that's a good point! I kept the old abis in case we used them elsewhere and was planning to delete them after I finished the gauntlet work and found that they could be deleted. They can definitely be deleted (modifying the schema.rs to just use the write_api!) and that would remove the extra "main" folder (just remember to modify the codegen.js file too), and this new logic calvin has to pull the execute, instantiate, and query abis out of there for all contracts. That'd be awesome.
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.
Thanks for the suggestions and clarifications! I've cleaned it up so that we only generate and use the full abi json 👍
@@ -15,7 +15,7 @@ export const enum CATEGORIES { | |||
} | |||
|
|||
export const DEFAULT_RELEASE_VERSION = 'local' | |||
export const DEFAULT_CWPLUS_VERSION = 'v0.16.0' | |||
export const DEFAULT_CWPLUS_VERSION = 'v1.1.0' |
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.
there's a single invocation of CosmWasmContract afaict:
this.contracts[id] = new CosmWasmContract(id, dirName) |
which does not provide the defaultVersion
(3rd) parameter at all:
constructor(id, dirName, defaultVersion = DEFAULT_CWPLUS_VERSION) { |
which means that version is always v1.1.0
and never local
, and this clause never gets invoked for CWPLUS i think?
if (version === 'local') { |
in which case, if it always gets downloaded, i wonder why we need to download it in the Makefile changes above? could we simply remove it? or, add it to our repo locally and remove the js clause so we don't depend on a download from github during deploy? wdyt?
Lines 74 to 79 in fd629af
contracts_install: artifacts_curl_deps artifacts_cp_gauntlet artifacts_cp_wasmd | |
artifacts_curl_deps: artifacts_curl_cw20 | |
artifacts_curl_cw20: | |
curl -Lo artifacts/cw20_base.wasm https://github.com/CosmWasm/cw-plus/releases/download/v0.8.0/cw20_base.wasm |
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.
good catch! you're right it seems to be unused - i've removed the curl in the makefile (tests still run fine)
@@ -1,6 +1,6 @@ | |||
{ | |||
"contract_name": "cw20-base", | |||
"contract_version": "0.16.0", | |||
"contract_version": "1.1.0", |
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.
is it strange that we download the contract each time, but we have the contract schema locally? 😄
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.
thats a good point actually - changed loadContractABI to fetch the abi from github as well. We can now remove all the local artifacts related to cw+ 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.
actually - had to restore the locally stored artifacts as it looks like codegen.js in the gauntlet tests rely on these
@@ -18,22 +18,4 @@ fn main() { | |||
execute: ExecuteMsg, | |||
query: QueryMsg, | |||
} | |||
|
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.
🥳
https://smartcontract-it.atlassian.net/browse/BCI-1469
Update CW Plus contract versions to v1.1.0
Reviewers can ignore changes to the artifacts folders, which are downloaded from https://github.com/CosmWasm/cw-plus/releases/tag/v1.1.0