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

test: fix e2e and build issues (DO NOT REVIEW) #4031

Closed

Conversation

charleenfei
Copy link
Contributor

Description

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

syedc5 and others added 3 commits July 6, 2023 20:59
- Update footer links to add in CometBFT docs, and label Tendermint Core docs as archived (should probably still be linked to for a while still)
- Changed youtube link to https://www.youtube.com/@interchain_io/featured. Cosmos youtube channel is no longer maintained
- Removed link to interchain.berlin. Added link to interchain.io

Co-authored-by: Carlos Rodriguez <[email protected]>
// InstantiateContractCosts costs when interacting with a wasm contract
func (g WasmGasRegister) InstantiateContractCosts(msgLen int) sdk.Gas {
if msgLen < 0 {
panic(sdkerrors.Wrap(ErrInvalid, "negative length"))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
func (g WasmGasRegister) ToWasmVMGas(source storetypes.Gas) uint64 {
x := source * g.c.GasMultiplier
if x < source {
panic(sdk.ErrorOutOfGas{Descriptor: "overflow"})

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
ctx.GasMeter().ConsumeGas(consumed, "wasm contract")
// throw OutOfGas error if we ran out (got exactly to zero due to better limit enforcing)
if ctx.GasMeter().IsOutOfGas() {
panic(sdk.ErrorOutOfGas{Descriptor: "Wasmer function execution"})

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
panic(err)
}

ctx := sdk.NewContext(nil, tmproto.Header{Height: 1, Time: time.Now()}, true, nil) // context with infinite gas meter

Check warning

Code scanning / CodeQL

Calling the system time

Calling the system time may be a possible source of non-determinism
func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) []exported.Height {
header, ok := clientMsg.(*Header)
if !ok {
panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
// newStoreAdapter constructor
func newStoreAdapter(s sdk.KVStore) *storeAdapter {
if s == nil {
panic("store must not be nil")

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt

_, err := call[contractResult](ctx, clientStore, &cs, payload)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt

// safety checks before casting below
if height < 0 {
panic("Block height must never be negative")

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
nsec := ctx.BlockTime().UnixNano()
if nsec < 0 {
panic("Block (unix) time must never be negative ")

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@charleenfei charleenfei changed the base branch from main to feat/wasm-clients July 7, 2023 08:02
@@ -36,7 +36,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
go-arch: ['amd64', 'arm', 'arm64']
go-arch: ['amd64', 'arm']
Copy link
Contributor

@DimitrisJim DimitrisJim Jul 7, 2023

Choose a reason for hiding this comment

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

I think this matrix just needs to change unfortunately. Instead of cross compiling, compile using a runner that matches the arch. Didn't have time to check yesterday if mac-os runner actually has arm64 arch.

Copy link
Contributor

Choose a reason for hiding this comment

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

No linux arm support apparently: actions/runner-images#5631
Similarly, mac-os doesn't seem to be using the new M1 arm64 chip: github/roadmap#528

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: can build on arm64, wasn't using cross compiler correctly. Can't on arm32 since wasmvm can't support it due to Wasmer not supporting it.

The previous does apply if we'd like to run the tests on arm64 via github runners tho.

@charleenfei charleenfei changed the base branch from feat/wasm-clients to carlos/3975-make-separate-gomod-for-08-wasm July 7, 2023 09:23
@charleenfei charleenfei changed the title DO NOT REVIEW (test) test: fix e2e and build issues (DO NOT REVIEW) Jul 7, 2023
RUN make build
# Grab the wasmvm library in order to copy it into the final image.
RUN cp $(ldd build/simd | grep "libwasmvm" | awk '{ print $3 }') libwasmvm.so
Copy link
Contributor

@DimitrisJim DimitrisJim Jul 7, 2023

Choose a reason for hiding this comment

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

A question. Do we want to keep using multi-stage builds? This (and the copy in the next image on line 34) is required because libwasmvm is included in the wasmvm go lib (which we lose access to in the next stage).

Are the space savings worth the hassle?

Copy link
Contributor

@DimitrisJim DimitrisJim Jul 10, 2023

Choose a reason for hiding this comment

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

Note, they are worth it. Looking into #2442 though since that might offer benefits in reducing both size and linking issues like this.

rm unnecessary COPY

Revert "rm unnecessary COPY"

This reverts commit 5ed9782.
@DimitrisJim DimitrisJim deleted the charly/test branch December 18, 2023 13:36
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.

3 participants