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

feat: merge upstream 1.0.0 #65

Merged
merged 223 commits into from
Jun 21, 2022
Merged

feat: merge upstream 1.0.0 #65

merged 223 commits into from
Jun 21, 2022

Conversation

shiki-tak
Copy link
Contributor

Description

Closes #64

Types of changes

  • Bug fix (changes which fixes an issue)
  • New feature (changes which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • ETC (build, ci, docs, perf, refactor, style, test)

Checklist

@shiki-tak shiki-tak marked this pull request as draft June 2, 2022 10:54
@CLAassistant
Copy link

CLAassistant commented Jun 2, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 4 committers have signed the CLA.

✅ shiki-tak
❌ Deployer
❌ webmaster128
❌ ethanfrey


Deployer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@shiki-tak shiki-tak self-assigned this Jun 6, 2022
@shiki-tak shiki-tak marked this pull request as ready for review June 6, 2022 07:16
@loloicci
Copy link
Member

loloicci commented Jun 6, 2022

Could you modify commit titles only "fix"? (it. is ok to use force push)

@shiki-tak
Copy link
Contributor Author

Could you modify commit titles only "fix"? (it. is ok to use force push)

Oops...I had forgotten to put together my commits, thx.

api/callbacks.go Outdated
// TODO figure out how to pass the text in its `Descriptor` field through all the FFI
// TODO handle these cases on the Rust side in the first place
// These three types are "thrown" (which is not a thing in Go 🙃) in panics from the gas module
// (https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/store/types/gas.go):
Copy link
Member

Choose a reason for hiding this comment

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

replace link to ours (if needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

api/callbacks.go Outdated
Comment on lines 76 to 77
// - https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/baseapp/baseapp.go#L607
// - https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/baseapp/recovery.go#L50-L60
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

api/callbacks.go Outdated
// This turns the panic into a regular error with a helpful error message.
//
// The other two gas related panic types indicate programming errors and are handled along
// with all other errors in https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/baseapp/recovery.go#L66-L77.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

api/callbacks.go Outdated
@@ -207,16 +226,16 @@ func cGet(ptr *C.db_t, gasMeter *C.gas_meter_t, usedGas *cu64, key C.U8SliceView
// https://github.com/line/lfb-sdk/blob/786df84b8e0aaa0a1aff79ffbab0541e597ee004/store/types/store.go#L203
Copy link
Member

Choose a reason for hiding this comment

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

"lfb-sdk" left here (Maybe not about this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thx!

@@ -10,6 +10,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/line/wasmvm/types"
dbm "github.com/tendermint/tm-db"
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? We removed the dependency to dbm with #15. Can it remove the dependency in this test?

Copy link
Member

Choose a reason for hiding this comment

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

If we don't need it, let's remove it from go.mod.


.PHONY: docker-image-alpine
docker-image-alpine:
docker build . -t $(BUILDERS_PREFIX):alpine -f ./Dockerfile.alpine
docker build --pull . -t $(BUILDERS_PREFIX):alpine -f ./Dockerfile.alpine

.PHONY: docker-image-static
docker-image-static:
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it needed update this line and Dockefile.static itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

types/queries.go Outdated
// StargateQuery is encoded the same way as abci_query, with path and protobuf encoded request data.
// The format is defined in [ADR-21](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-021-protobuf-query-encoding.md).
// The response is protobuf encoded data directly without a JSON response wrapper.
// The caller is responsible for compiling the proper protobuf definitions for both requests and responses.
type StargateQuery struct {
// this is the fully qualified service path used for routing,
// eg. custom/lfb_sdk.x.bank.v1.Query/QueryBalance
Copy link
Member

Choose a reason for hiding this comment

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

lfb -> lbm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thx!

@@ -10,6 +10,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/line/wasmvm/types"
dbm "github.com/tendermint/tm-db"
Copy link
Member

Choose a reason for hiding this comment

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

If we don't need it, let's remove it from go.mod.

go.mod Outdated Show resolved Hide resolved
@@ -9,7 +9,7 @@ jobs:
- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: 1.54.0
toolchain: 1.59.0
Copy link
Member

Choose a reason for hiding this comment

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

Is it no problem?
I heard the appropriate rustc version is 1.57.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx! I changed to 1.57.0

Copy link
Member

Choose a reason for hiding this comment

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

Now, the toolchain version of Dockerfiles is 1.60.0. If there are no reasons we cannot unify, we should do.

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@loloicci loloicci left a comment

Choose a reason for hiding this comment

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

Rust toolchain versions of Dockerfiles' are 1.60.0 and github workflows' are 1.57.0. If there are no reason we cannot unify, we should do.

@shiki-tak shiki-tak merged commit bd8950c into Finschia:main Jun 21, 2022
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.

Migrate v1.0.0
8 participants