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

sdk 047 #1132

Closed
wants to merge 44 commits into from
Closed

sdk 047 #1132

wants to merge 44 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Dec 18, 2022

Hi, @alpe -- here's a PR into the 47 branch you're working on.

If you'd rather not get these, please let me know.

Cheers!

-Jacob

Thus far, this PR:

  • fixes lint issues
  • fixes benchmark tests

@faddat faddat requested a review from alpe as a code owner December 18, 2022 14:14
app/ante.go Fixed Show fixed Hide fixed
@alpe
Copy link
Contributor

alpe commented Dec 19, 2022

Thanks Jacob! I appreciate your help. It is easy to miss some changes so any help is appreciated but let's stick with official repos. We are in close contact with the IBC team, too to get any issues out of our way. Status of inter-tx is one of them.

@faddat
Copy link
Contributor Author

faddat commented Dec 19, 2022

totally :). think we've got a PR into ibc-go for the ibc, as I fully agree re: official repos. Basically that replace is there so we could make progress.

cosmos/ibc-go#2955

Next tho, concerning icad, I think that there are some questions as to how it's doing ICA, post ibc v6.

There were some kinda questionable-feeling pieces of the code when I read through it.

Thanks to yall at confio for your work over the years!

Vuong, thx for the cleanup on these :)

alpe and others added 3 commits December 21, 2022 10:30
(cherry picked from commit f7f8417)
(cherry picked from commit 35b2a8f)
Copy link
Contributor

@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.

Thank you for your submission again! I have cherry picked some commits to my development branch. I appreciate the help! 🏅

@@ -39,11 +39,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
return nil, sdkerrors.Wrap(sdkerrors.ErrLogic, "tx counter key is required for ante builder")
}

sigGasConsumer := options.SigGasConsumer
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good eye

@@ -497,8 +497,8 @@ func (chain *TestChain) CreateTMClientHeader(chainID string, blockHeight int64,
// MakeCommit expects a signer array in the same order as the validator array.
// Thus we iterate over the ordered validator set and construct a signer array
// from the signer map in the same order.
signerArr := make([]tmtypes.PrivValidator, len(tmValSet.Validators))
for i, v := range tmValSet.Validators {
signerArr := make([]tmtypes.PrivValidator, len(tmValSet.Validators)) //nolint:staticcheck // TODO: determine if there's really a possible nil pointer dereference
Copy link
Contributor

Choose a reason for hiding this comment

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

@faddat Are you going to follow up on this? Let's not add more todos in code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yeah, will be following up now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: wdyt on this one?

Now that I'm looking at it, the TODO is def because I wasn't real sure at all.

.circleci/config.yml Outdated Show resolved Hide resolved
app/app.go Outdated
@@ -127,9 +127,9 @@ import (

// Note: please do your research before using this in production app, this is a demo and not an officially
// supported IBC team implementation. It has no known issues, but do your own research before using it.
// intertx "github.com/cosmos/interchain-accounts/x/inter-tx"
Copy link
Contributor

Choose a reason for hiding this comment

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

I will keep inter-tx commented out until official supported versions are available. This is just a TODO

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@alpe
Copy link
Contributor

alpe commented Jan 9, 2023

Closing this in favour of #1136 . I would be happy to get your review

@alpe alpe closed this Jan 9, 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.

3 participants