Closed aljo242 closed 2 years ago
Merging #914 (bfdf277) into develop (e8a2f9c) will decrease coverage by
0.00%
. The diff coverage is8.16%
.
@@ Coverage Diff @@
## develop #914 +/- ##
===========================================
- Coverage 10.56% 10.55% -0.01%
===========================================
Files 327 327
Lines 75191 75202 +11
===========================================
- Hits 7944 7941 -3
- Misses 67059 67072 +13
- Partials 188 189 +1
Impacted Files | Coverage Ξ | |
---|---|---|
pkg/types/consensus_state.go | 100.00% <ΓΈ> (ΓΈ) |
|
pkg/types/monitoring.pb.go | 0.79% <ΓΈ> (ΓΈ) |
|
pkg/types/validator_set.go | 92.30% <ΓΈ> (ΓΈ) |
|
x/campaign/types/events.pb.go | 0.55% <ΓΈ> (ΓΈ) |
|
x/campaign/types/mainnet_account.pb.go | 0.75% <ΓΈ> (ΓΈ) |
|
x/campaign/types/params.pb.go | 0.88% <ΓΈ> (ΓΈ) |
|
x/campaign/types/query.pb.go | 0.66% <ΓΈ> (ΓΈ) |
|
x/claim/keeper/mission_delegation_hooks.go | 0.00% <0.00%> (ΓΈ) |
|
x/claim/types/claim_record.pb.go | 0.83% <ΓΈ> (ΓΈ) |
|
x/claim/types/mission.pb.go | 1.01% <ΓΈ> (ΓΈ) |
|
... and 35 more |
What difference with https://github.com/tendermint/spn/pull/906?
Is it with the final imports for IBC and fundraising?
What difference with #906?
That PR included all of the changes that I made in the previous PRs to prepare for v0.46.0
. It would have been a mess to review. I have closed #906 now.
This PR also fixes tests that were broken by the upgrade.
Is it with the final imports for IBC and fundraising?
These are the final imports for a temporary solution. My update to fundraising
works, but I'm just waiting for them to approve. We will still need to make a PR later that removes the replace
lines in the go.mod
once the dependencies are fixed.
I'm expected ignite serve
to fail currently but not ignite g proto-go
Seems like there are some changes for proto
cosmos/base/v1beta1/coin.proto:32:8: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/base/v1beta1/coin.proto:37:19: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/base/v1beta1/coin.proto:42:19: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
campaign/special_allocations.proto:7:1: Import "cosmos/base/v1beta1/coin.proto" was not found or had errors.
campaign/special_allocations.proto:10:12: "cosmos.base.v1beta1.Coin" is not defined.
campaign/special_allocations.proto:15:12: "cosmos.base.v1beta1.Coin" is not defined.
campaign/campaign.proto:7:1: Import "cosmos/base/v1beta1/coin.proto" was not found or had errors.
campaign/campaign.proto:8:1: Import "campaign/special_allocations.proto" was not found or had errors.
campaign/campaign.proto:17:12: "cosmos.base.v1beta1.Coin" is not defined.
campaign/campaign.proto:22:12: "cosmos.base.v1beta1.Coin" is not defined.
campaign/campaign.proto:27:3: "SpecialAllocations" is not defined.
There is some dependency on the cosmos-proto
package now. I think that the ignite
command may need to be modified to accommodate that
If you run
go get github.com/regen-network/cosmos-proto@latest
ignite chain serve --proto-all-modules --clear-cache --force-reset -v
proto compilation works for me. Does this work for you as well?
https://github.com/cosmos/cosmos-sdk/pull/11452/files this PR might be helpful
If you run
go get github.com/regen-network/cosmos-proto@latest ignite chain serve --proto-all-modules --clear-cache --force-reset -v
proto compilation works for me. Does this work for you as well?
Nope, I have the same error
Supporting the proto changes will be part of https://github.com/ignite/cli/issues/2156
Supporting the proto changes will be part of ignite/cli#2156
Sounds good. If that's the only outstanding issue, are we good to try to get this approved?
Supporting the proto changes will be part of ignite/cli#2156
Sounds good. If that's the only outstanding issue, are we good to try to get this approved?
I think this is better if we can merge it once we can run it with ignite c serve
and use ignite c build
.
If we need the new version for spn
dependency in ignite
, maybe we can temporarily update spn
dep in ignite
to this branch?
Supporting the proto changes will be part of ignite/cli#2156
Sounds good. If that's the only outstanding issue, are we good to try to get this approved?
I think this is better if we can merge it once we can run it with
ignite c serve
and useignite c build
.If we need the new version for
spn
dependency inignite
, maybe we can temporarily updatespn
dep inignite
to this branch?
I think it would be better the other way around. If this upgrade fully works, then the only remaining issue is with ignite
. So it makes more sense to upgrade spn
, then fix ignite
after.
After all, we have now modified our dependencies so that:
ignite
imports spn
spn
no longer imports ignite
What does it change if we do go get github.com/tendermint/spn@6bff6689d24b74fd3d6c2786db475e5250708ef9
on Ignite?
This will remove the circular dep, then we just need to update again spn
dep on Ignite once https://github.com/ignite/cli/issues/2673 is finished.
I can't test everything on the changes without using Ignite
What does it change if we do
go get github.com/tendermint/spn@6bff6689d24b74fd3d6c2786db475e5250708ef9
on Ignite?This will remove the circular dep, then we just need to update again
spn
dep on Ignite once ignite/cli#2673 is finished. I can't test everything on the changes without using Ignite
Good point - I was thinking this would introduce a fork in the go.mod
but my branch is on here. I'll go ahead and do that.
There could be one problem with that because that commit hash wouldn't be guaranteed to be kept forever on Github.
The reason for this, once we merge this PR, all the commits here will be squashed into a single commit and the commit that has mentioned will not have a reference on develop PR and also in the base branch of this PR since the branch will be deleted after merging.
I suggest merging this PR first and using the commit hash added to the develop branch instead.
But if we plan to change the commit hash again after we finish work then np.
Ah - I see what you mean. I just need some approvals and we can merge
There could be one problem with that because that commit hash wouldn't be guaranteed to be kept forever on Github.
The reason for this, once we merge this PR, all the commits here will be squashed into a single commit and the commit that has mentioned will not have a reference on develop PR and also in the base branch of this PR since the branch will be deleted after merging.
I suggest merging this PR first and using the commit hash added to the develop branch instead.
It will be temporary, the time the serve
command works CLI side for 0.46
we don't need to wait for the full 0.46
PR to be merged CLI side
And as soon as this PR is merged we can change it
In that case there should be no problem. π
Updates the Cosmos SDK to
v0.46.0
. Please review this PR after the other onces I have submitted are completed.This PR builds off of those, and will be much more manageable to review if the other PRs are already merged.
This PR introduces temporary
replace
clauses in ourgo.mod
for the following packages:github.com/cosmos/ibc-go/v5 => github.com/aljo242/ibc-go/v5 v5.1.0
github.com/tendermint/fundraising => github.com/aljo242/fundraising v0.1.2-0.20220726203752-c39abf16b02d
Once the respective repos are updated, we can remove these lines. I have an open PR for
x/fundraising
andibc-go
is near completion.Part of #909
Checklist
These are the criteria that every PR should meet, please check them off as you review them: