Skip to content

Conversation

yihuang
Copy link

@yihuang yihuang commented Aug 28, 2025

Description

Backport #8573 without api breakage.


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.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Include changelog entry when appropriate (e.g. chores should be omitted from changelog).
  • Wrote unit and integration tests if relevant.
  • Updated documentation (docs/) if anything is changed.
  • Added godoc comments if relevant.
  • Self-reviewed Files changed in the GitHub PR explorer.
  • Provide a conventional commit message to follow the repository standards.

)

* imp: added address codec to transfer

* imp: address codec used in transfer

* fix: tests

* imp: test passing

* imp: added addressCodec to PFM

* imp: added receiver test

* test: transfer cov

* test: added cases

* test: more cov

* test: more cov

* style: single line func def

* test: new test works

* imp: add basic test

* imp: added migration guide

* doc: added changelog

* refactor: address codec moved
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/v10.4.x@0b90af4). Learn more about missing BASE report.

Files with missing lines Patch % Lines
modules/apps/transfer/keeper/relay.go 66.66% 1 Missing ⚠️
modules/apps/transfer/v2/ibc_module.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##             release/v10.4.x    #8615   +/-   ##
==================================================
  Coverage                   ?   62.55%           
==================================================
  Files                      ?      288           
  Lines                      ?    20794           
  Branches                   ?        0           
==================================================
  Hits                       ?    13008           
  Misses                     ?     7224           
  Partials                   ?      562           
Flag Coverage Δ
08-wasm 66.28% <ø> (?)
e2e 1.18% <ø> (?)
ibc-go 68.98% <91.30%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gjermundgaraba
Copy link
Contributor

It looked like CI was having trouble running the E2Es, so I just copied it into a separate PR to run it there: #8616 (I'll close that one once it's green)

@gjermundgaraba
Copy link
Contributor

The lint issue would be good to get fixed tho

@yihuang
Copy link
Author

yihuang commented Aug 29, 2025

The lint issue would be good to get fixed tho

fixed.

@gjermundgaraba gjermundgaraba changed the base branch from release/v10.3.x to release/v10.4.x August 29, 2025 10:13
Comment on lines +96 to +105
// SetAddressCodec sets the address codec used by the keeper.
func (k *Keeper) SetAddressCodec(addressCodec address.Codec) {
k.addressCodec = addressCodec
}

// GetAddressCodec returns the address codec used by the keeper.
func (k *Keeper) GetAddressCodec() address.Codec {
return k.addressCodec
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you instantiate address codec in NewKeeper with a default SDK keeper? In case they forget to call SetAddressCodec?

I see you opted to use k.addressCodec != nil in line. Is there a reason for this?

Copy link
Author

Choose a reason for hiding this comment

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

this backport is to avoid API breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what he meant was that inside the NewKeeper function, you should set up a default address codec, so that someone not changing their app.go to include the SetAddressCodec would not break.

Copy link
Author

Choose a reason for hiding this comment

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

ah, sounds good, changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks 🙏

Copy link
Contributor

@gjermundgaraba gjermundgaraba left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +96 to +105
// SetAddressCodec sets the address codec used by the keeper.
func (k *Keeper) SetAddressCodec(addressCodec address.Codec) {
k.addressCodec = addressCodec
}

// GetAddressCodec returns the address codec used by the keeper.
func (k *Keeper) GetAddressCodec() address.Codec {
return k.addressCodec
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks 🙏

@gjermundgaraba gjermundgaraba merged commit 8c66bc6 into cosmos:release/v10.4.x Sep 1, 2025
15 of 16 checks passed
@yihuang yihuang deleted the backport-8573 branch September 2, 2025 00:59
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