Skip to content
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

Coral binary #223

Merged
merged 4 commits into from
Jul 27, 2020
Merged

Coral binary #223

merged 4 commits into from
Jul 27, 2020

Conversation

ethanfrey
Copy link
Member

Configure wasmd/wasmcli binaries for the coral testnet.

We use the same code, just set a few flags in the Makefile to set a unique bech32 prefix, directory, and node names.

Try make build-coral and try it out - eg look at keys, see config is different than wasm, look at help

Anything else I need to customize?

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added relevant godoc comments.

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Re-reviewed Files changed in the Github PR explorer


For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@ethanfrey ethanfrey requested a review from alpe July 27, 2020 09:15
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #223 into master will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   72.41%   72.26%   -0.16%     
==========================================
  Files          27       27              
  Lines        2632     2632              
==========================================
- Hits         1906     1902       -4     
- Misses        614      617       +3     
- Partials      112      113       +1     
Impacted Files Coverage Δ
app/app.go 89.34% <ø> (ø)
lcd_test/helpers.go 75.00% <0.00%> (-0.72%) ⬇️
x/wasm/internal/keeper/keeper.go 91.15% <0.00%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 826ca87...cd9ae04. Read the comment docs.

@orkunkl
Copy link
Contributor

orkunkl commented Jul 27, 2020

A suggestion: instead of merging to master, we can create a branch testnets/coral and apply changes there, thus provide a binary via testnet-release?

@orkunkl orkunkl mentioned this pull request Jul 27, 2020
2 tasks
@ethanfrey
Copy link
Member Author

testnet-release is gone as of #221

I would prefer to limit branches, as they are a headache to maintain. Which version of corald corresponds to which version of wasmd and how to tag?

My attempt was to add multiple binaries without multiple cmd packages via the Makefile. This probably looks cleaner after #221

Makefile Show resolved Hide resolved
Copy link
Member

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested some cli commands to work with keys and to instantiate a contract. All looked 👌 so far.

  • Home directories ✅
  • Key creation ✅
  • In contract bech32 decoding ✅
  • Cosmos addr in contract decoding wasm contract failed: Error calling the VM: Calling external function through FFI: Unknown error during FFI call: Some(\"invalid Bech32 prefix; expected coral, got cosmos\")
  • Contract queries ✅

@ethanfrey
Copy link
Member Author

Cool, thank you fro the detailed testing.

I did much of that, but did not Cosmos addr in contract decoding. Cool to get the detailed checks.

@ethanfrey
Copy link
Member Author

A bit out of context, but what do you think of this error message @alpe @webmaster128

wasm contract failed: Error calling the VM: Calling external function through FFI: Unknown error during FFI call: Some(\"invalid Bech32 prefix; expected coral, got cosmos\")

It contains the original error message and all context, but is maybe a bit too verbose. There is a bit of a stutter there. I guess this would be more ideal:

wasm contract failed: Error calling the VM: Unknown error during FFI call: \"invalid Bech32 prefix; expected coral, got cosmos\"

We can make a (not so high priority) issue on cosmwasm if you like that or have idea for a better one

@alpe
Copy link
Member

alpe commented Jul 27, 2020

A bit out of context, but what do you think of this error message @alpe @webmaster128

wasm contract failed: Error calling the VM: Calling external function through FFI: Unknown error during FFI call: Some(\"invalid Bech32 prefix; expected coral, got cosmos\")

It contains the original error message and all context, but is maybe a bit too verbose. There is a bit of a stutter there. I guess this would be more ideal:

wasm contract failed: Error calling the VM: Unknown error during FFI call: \"invalid Bech32 prefix; expected coral, got cosmos\"

We can make a (not so high priority) issue on cosmwasm if you like that or have idea for a better one

We can always aim for better UX but let's confirm first this is a real issue for users. It did not hurt :-)

@webmaster128
Copy link
Member

This error will change completely when passed back to the contract (CosmWasm/wasmvm#124) and propagates as a StdError instead of a VmError.

@ethanfrey
Copy link
Member Author

Good point @webmaster128

Let's review these again when all last changes are made.

@ethanfrey ethanfrey merged commit b269f78 into master Jul 27, 2020
@ethanfrey ethanfrey deleted the coral-binary branch July 27, 2020 13:45
@gelinger777
Copy link

@ethanfrey I am a bit confused. I have in linux corald in command line but not coral.

And in your docs to setup enviornment here
https://docs.cosmwasm.com/getting-started/setting-env.html

you have

coral config chain-id $CHAIN_ID
coral config trust-node true

and coral not found .

Should I replace in that doc all coral to corald?

@ethanfrey
Copy link
Member Author

Yeah, please add an issue to docs

With the statgate upgrade, we merged them into one binary to track the sdk patterns. That should always be corald, wasmd, etc for both client and server now.

@ethanfrey
Copy link
Member Author

Also config command no longer works in stargate (this is due to sdk changes) and those must be passed down as flags.

The docs are correct up to v0.11. @orkunkl can you split those docs into 2 sections for launchpad and stargate?

@gelinger777
Copy link

can you hint here how to use the testchain ?

what will be equivalent of this code in docs?

coral config chain-id $CHAIN_ID
coral config trust-node true

# if connecting to local node: coral config node http://localhost:26657
coral config node $RPC
coral config output json
coral config indent true

# this is important, so the cli returns after the tx is in a block,
# and subsequent queries return the proper results
coral config broadcast-mode block

# check you can connect
coral query supply total
coral query staking validators
coral query wasm list-code

# add more wallets for testing
coral keys add fred
>
{
  "name": "fred",
  "type": "local",
  "address": "coral1avdvl5aje3zt0uay40uj6l9xtqtlqhduu84nql",
  "pubkey": "coralpub1addwnpepqvcjveqepq34x59fnmygdy58ag7zwu8gefgsprq9th38nxzptpgszc3rkve",
  "mnemonic": "hobby bunker rotate piano satoshi planet network verify else market spring toward pledge turkey tip slim word jaguar congress thumb flag project chalk inspire"
}

coral keys add bob
coral keys add thief

@alpe
Copy link
Member

alpe commented Dec 14, 2020

@gelinger777 coralnet was deprecated and is not running anymore AFAIK. I will add an issue cleanup the Makefile.
If you would join our #testnet channel on discord we can help you get up to speed with musselnet

zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants