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.

Sign up for GitHub

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

Jump to bottom

feat(osmosis-outpost): modify Osmosis outpost to use cross-chain contract V1 #2077

Merged
merged 34 commits into from
Nov 29, 2023
Merged

feat(osmosis-outpost): modify Osmosis outpost to use cross-chain contract V1 #2077

merged 34 commits into from
Nov 29, 2023

Conversation

0xstepit
Copy link
Contributor

@0xstepit 0xstepit commented Nov 27, 2023 •
edited

This PR updates the Osmosis outpost to reflect the new specs requirements. Specifically, the PR aims to:

  • Add wasm field in the memo (yeah, it was missing).
  • Add a function to convert aevmos and uosmo in Osmosi denom representation. To do so the channel keeper has been added to the precompile.
  • Fix typos on docstring.
  • Update docstrings to reflect new specs.

Missing:

  • : mainnet and testnet xcsv1 contract addresses. Mainnet is empty string and instantiation of the precompile raise an error if not changed in mainnet.

@codecov Codecov
Copy link

codecov bot commented Nov 27, 2023 •
edited

Codecov Report

Merging #2077 (7e86f5a) into main (f68bd3e) will increase coverage by 0.02%.
The diff coverage is 92.30%.

❗ Current head 7e86f5a differs from pull request most recent head d8b5fa1. Consider uploading reports for the commit d8b5fa1 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2077      +/-   ##
==========================================
+ Coverage   69.00%   69.02%   +0.02%     
==========================================
  Files         301      301              
  Lines       23573    23638      +65     
==========================================
+ Hits        16266    16317      +51     
- Misses       6423     6433      +10     
- Partials      884      888       +4
Files Coverage Δ
precompiles/outposts/osmosis/events.go 65.71% <100.00%> (ø)
precompiles/outposts/osmosis/osmosis.go 41.42% <100.00%> (+4.50%) ⬆️
precompiles/outposts/osmosis/tx.go 83.92% <100.00%> (-3.96%) ⬇️
precompiles/outposts/osmosis/types.go 89.24% <88.52%> (-2.66%) ⬇️

@0xstepit 0xstepit marked this pull request as ready for review November 28, 2023 11:52
@0xstepit 0xstepit requested a review from a team as a code owner November 28, 2023 11:52
@0xstepit 0xstepit requested review from MalteHerrmann and GAtom22 and removed request for a team November 28, 2023 11:52
GAtom22
GAtom22 requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Great work @0xstepit !! Left a few comments

Please address the lint issues

precompiles/outposts/osmosis/types_test.go Show resolved Hide resolved
@0xstepit 0xstepit marked this pull request as draft November 28, 2023 12:49
@0xstepit 0xstepit marked this pull request as ready for review November 28, 2023 15:04
fedekunze
fedekunze requested changes Nov 29, 2023
View reviewed changes
precompiles/outposts/osmosis/types.go Outdated Show resolved Hide resolved
precompiles/outposts/osmosis/types.go Outdated Show resolved Hide resolved
precompiles/outposts/osmosis/types.go Outdated Show resolved Hide resolved
fedekunze
fedekunze approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

minor comments

precompiles/outposts/osmosis/tx.go Outdated Show resolved Hide resolved
precompiles/outposts/osmosis/types.go Outdated Show resolved Hide resolved
precompiles/outposts/osmosis/types.go Outdated Show resolved Hide resolved
precompiles/outposts/osmosis/types_test.go Outdated Show resolved Hide resolved
GAtom22
GAtom22 approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @0xstepit!!

@fedekunze fedekunze enabled auto-merge (squash) November 29, 2023 13:00
Vvaradinov
Vvaradinov reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! Just wanted to note that valid params for the 2 cases are:

  • Evmos for Osmo - aevmos / uosmo
  • Osmo for Evmos - IBCDenom / IBCDenom

Seems the function for converting is taking the IBC denom for uosmo for the case where we send aevmos ?

@fedekunze fedekunze merged commit 52949ce into main Nov 29, 2023
28 of 29 checks passed
@fedekunze fedekunze deleted the stepit/osmosis-outpost-v1 branch November 29, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vvaradinov Vvaradinov Vvaradinov left review comments

@fedekunze fedekunze fedekunze approved these changes

@GAtom22 GAtom22 GAtom22 approved these changes

@MalteHerrmann MalteHerrmann Awaiting requested review from MalteHerrmann MalteHerrmann is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@0xstepit @Vvaradinov @fedekunze @GAtom22