Skip to content

deps: [DO NOT MERGE] v0.50.x integration CI tests #3

Closed
julienrbrt wants to merge 117 commits intomainfrom
sdk-v0.50.x
Closed

deps: [DO NOT MERGE] v0.50.x integration CI tests #3
julienrbrt wants to merge 117 commits intomainfrom
sdk-v0.50.x

Conversation

@julienrbrt
Copy link
Collaborator

This PR is only meant to see CI in the fork

@github-actions
Copy link

github-actions bot commented Feb 7, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://01builders.github.io/celestia-app/pr-preview/pr-3/

Built to branch gh-pages at 2025-03-10 12:00 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

julienrbrt and others added 20 commits February 7, 2025 12:29
* chore: re-wiring app.go to use pfm module on ibc-go v9

* Update app/app.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

---------

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
…ts (#6)

* refactor(x/tokenfilter): update to support ics20 v2 packet data packets

* chore: update comment
…eTxs) (#9)

* chore: fix more build errors (add addr -> bech32, refactor user CreateTx)

* revert

* updates
* chore: downgrading to ibc-go v8.5.2 and pfm v8.1.0

* chore: add call to WithQueryRouter
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR integrates the v0.50.x CI tests in a fork and updates the codebase to work with the latest module versions (e.g. migrating from v3 to v4 of celestia-app, and from tendermint to cometbft libraries). Key changes include configuration updates in .golangci.yml, refactoring of test cases to reflect new API signatures and response formats, and removal of deprecated modules (e.g. msg gatekeeper and app address definitions).

Reviewed Changes

File Description
.golangci.yml Added configuration for the new gci linter and build-tags are commented out.
app/app_test.go Updated import paths and adjusted test cases for baseapp sealing and InitChain behavior.
app/ante/gov.go Refactored GovProposalDecorator with added nested msg checks and TODO placeholders for unmarshalling.
Benchmarks files (benchmark_msg_send_test.go, benchmark_ibc_update_client_test.go) Updated transaction handling to new APIs and response formats, but note potential issues in per-transaction gas summing.
Various ante and fee tests Migrated imports from v3 to v4 and updated use of decoding and parameter types.
Removed files (app/address.go, msg_gatekeeper.go, msg_gatekeeper_test.go) Legacy support removed in favor of updated implementations.

Copilot reviewed 366 out of 366 changed files in this pull request and generated 2 comments.

totalGas += resp.GasUsed
require.Equal(b, uint32(0), resp.TxResults[i].Code)
require.Equal(b, "", resp.TxResults[i].Codespace)
totalGas += resp.TxResults[0].GasUsed
Copy link

Copilot AI Mar 3, 2025

Choose a reason for hiding this comment

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

In the loop that iterates over rawTxs, the gas consumption is always accumulated from resp.TxResults[0] instead of using the index 'i'. This may underreport the gas usage if there are multiple transactions; please change to resp.TxResults[i].GasUsed.

Suggested change
totalGas += resp.TxResults[0].GasUsed
totalGas += resp.TxResults[i].GasUsed

Copilot uses AI. Check for mistakes.
app/ante/gov.go Outdated
Comment on lines +56 to +57
// todo unmarshal

Copy link

Copilot AI Mar 3, 2025

Choose a reason for hiding this comment

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

The TODO comment indicates that nested message unmarshalling is not yet implemented in checkNestedMsgs; this could lead to skipped parameter checks. Please implement the unmarshalling logic to properly inspect and validate the nested messages.

Suggested change
// todo unmarshal
if err := gogoproto.Unmarshal(msg.Value, exec); err != nil {
return errors.Wrap(err, "failed to unmarshal authz.MsgExec")
}

Copilot uses AI. Check for mistakes.
chatton and others added 4 commits March 3, 2025 12:46
* chore: fix share proof test

* chore: bump to latest comet version
* test: fix ledger test errors

* chore: update sdk go mod
@damiannolan
Copy link

damiannolan commented Mar 4, 2025

Tests are now passing modulo the skipped tests (will comb through and create issues for following up - I think some require discussion on PR with core/app team - some require adaptations and such).

Some of the remaining items I have from memory are:

  • Reenable priority test when support has been reimplemented in core (CheckTxRes prioritisation).
  • Refactor inflation test in x/mint (we no longer have the ability to manipulate block times by overriding a param which no longer exists in tmConfig).
  • Cleanup and renable tests from paramfilter module removal - should be replaced by gov ante handler (unfinished).
  • Decide on removal of param tests in x/blob (think were only required for previous AppVersion conditionals)
  • Follow up on commented out legacy tests in x/signal due to usage of x/circuit and x/gov msg execution (different expectation of when something fails now).
  • Reenable commented out TestSquareSizeUpperBound (in-progress)
  • Check remaining TODOs

@damiannolan damiannolan changed the title [DO NOT MERGE] v0.50.x integration CI tests deps: [DO NOT MERGE] v0.50.x integration CI tests Mar 4, 2025
Copy link

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

so much tech debt removed via updating and not doing all the if statement version gates!!! thanks so so much for this

what's the best way to learn to the differences in how the sdk changed wiring the application together in v0.50? I tried looking at the changelogs etc but it was difficult to find what I was looking for

I don't have a ton of comments due to the massive change but will come back after chewing a bit. Mainly just questions

Comment on lines -17 to +18
DefaultNetworkMinGasPrice sdk.Dec
DefaultNetworkMinGasPrice math.LegacyDec

Choose a reason for hiding this comment

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

out of curiosity, what does switching to the new dec entail? presumably we're not switching now due to it requiring a large migration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The type was moved from the SDK to cosmossdk.io/math. No behavior change was involved, only any API break 😅

Comment on lines -11 to -21
// wrapEndBlocker overrides the app's endblocker to set the timeout commit to a
// different value for testnode.
func wrapEndBlocker(app *app.App, timeoutCommit time.Duration) func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
endBlocker := func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
resp := app.EndBlocker(ctx, req)
resp.Timeouts.TimeoutCommit = timeoutCommit
return resp
}

return endBlocker
}

Choose a reason for hiding this comment

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

what's the mechanism for doing this now?

Comment on lines +505 to +508
// TODO: check if needed, it is no more there
// res.Timeouts.TimeoutCommit = app.getTimeoutCommit(currentVersion)
// res.Timeouts.TimeoutPropose = appconsts.GetTimeoutPropose(currentVersion)
return res, nil

Choose a reason for hiding this comment

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

is there an alternative mechanism to change the timeouts in a rolling fashion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to dig a bit more into this one, will try and investigate this today!

Choose a reason for hiding this comment

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

I don't have full context on why these were removed. Were the additions of these to State in core done specifically by the celestia-core fork?

As I understand these are config.toml configurations in tm, and could be set by a node op so you may end up with differing configurations node by node (?)

Is the intention for timeout commit to be controlled by the state machine so all validators are aligned?

Choose a reason for hiding this comment

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

whoops, I fee like this invalidates my thinking above. If a node op starts with --timeout-commit then its set on the App itself and used first before some const.

func (app *App) getTimeoutCommit(appVersion uint64) time.Duration {
	if app.timeoutCommit != 0 {
		return app.timeoutCommit
	}
	return appconsts.GetTimeoutCommit(appVersion)
}

Comment on lines -577 to -579
res.Timeouts.TimeoutCommit = app.getTimeoutCommit(appVersion)
res.Timeouts.TimeoutPropose = appconsts.GetTimeoutPropose(appVersion)
return res

Choose a reason for hiding this comment

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

same question as above about adding the timeouts. My understanding is that we still need these but I could be wrong

@julienrbrt
Copy link
Collaborator Author

what's the best way to learn to the differences in how the sdk changed wiring the application together in v0.50? I tried looking at the changelogs etc but it was difficult to find what I was looking for

Best is to read the upgrade guides. The changelog is exhaustive while the upgrade guide only mentions the most important:

damiannolan and others added 5 commits March 6, 2025 13:59
* test: refactor square size upper bound tests

* chore: update sdk fork with fix

* test: finish refactor of square size test
#47)

* refactor: remove appVersion switches from prepare and process proposal

* chore: remove appVersion lookups in prepare and process prop
…icates (#48)

* chore: use params.BondDenom in favour of app.BondDenom to remove duplicates

* ci: fix incorrect field name in cov upload workflow
* chore: bump nova version

* chore: adding multiplexer test

* chore: add workflow dispatch

* chore: run on any PR

* chore: correct curl command

* chore: run tests in correct directory

* chore: add basic test in nova dir

* chore: adding first iteration of go test

* chore: cd into celestia-app before calling make install

* chore: fix compilation errors

* chore: passing test locally

* chore: update workflow

* chore: update workflow

* chore: update to relative path

* chore: change install to build

* chore: tweak naming in workflow

* chore: update branch

* chore: remove panic in NewPassthroughCmd

* chore: run linter, use lfs in workflow (final commit to test ci before updating branch to use sdk 50)

* chore: update branch to sdk one
* chore: remove more versioning checks

* Update app/ante/fee_checker.go

* chore: rename ante files, sweep setMinFee code to func

* chore: rm duplicate file
chatton and others added 9 commits March 7, 2025 12:20
* chore: bump nova

* chore: update go version

* chore: use 1.23.5 as is on main
* chore: adding embedded placeholder files to internal directory

* chore: correctly create v3 binary with embedded data

* chore: updating workflow to use binary embedded directly into the celestia-app

* chore: fixed typo in package name

* chore: fix linter

* chore: correct curl command

* chore: remove custom branch
… tests (#53)

* chore: remove offer snapshot version checking

* chore: removing some versioning logic in tests

* chore: lint
* refactor: protobuf make targets for v0.50

* chore: rm swagger until required

* chore: rm comment in clang format file

* chore: use buf raw tooling, rm protobuilder docker
* chore: remove offer snapshot version checking

* chore: removing some versioning logic in tests

* chore: lint

* chore: removing some tests around versioned consts

* chore: removed version from UpgradeHeightDelay and ran linter

* chore: remove several functions from the versioned app consts and replace with defaults

* chore: use defaults instead of accessing appv4 package

* chore: use appconsts instead of appv4

* chore: use appconsts everywhere
…pe (#55)

* Refactor blob_share_decorator_test to use appconsts.LatestVersion

* chore: removed appVersion from signer and removed appVersion from all usages of the Signer type

* chore: remove reference to app version in test

* chore: remove appVersion from max tx size decorator test

* chore: lint
* wip: added gov ante handler impl with param filters

* refactor: implement param filters for blocked gov params

* test: adding unit tests and docstrings for the param filters

* chore: re-wrote gov tests

* chore: update godocs

* chore: rename constructor func

* chore: renames and doc strings

---------

Co-authored-by: chatton <github.qpeyb@simplelogin.fr>
@damiannolan
Copy link

We can probs close this in favour of the upstream PR now and treat that as canonical

@julienrbrt julienrbrt closed this Mar 11, 2025
@julienrbrt
Copy link
Collaborator Author

🫡

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.

7 participants