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

imp(osmosis-outpost): Make the input to the swap function a struct and refactor tests #2206

Merged
merged 13 commits into from
Jan 2, 2024

Conversation

Vvaradinov
Copy link
Contributor

@Vvaradinov Vvaradinov commented Dec 19, 2023

Description


This PR creates a struct type for the Osmosis outpost input params to keep the validation and parsing cleaner. It also implements the Outpost Client Arguments ADR by passing extra params like channelID and XCSContract.

Closes XAP-10

@Vvaradinov Vvaradinov changed the title imp(osmosis-outpost): Make the input param to the swap function a struct and refactor tests imp(osmosis-outpost): Make the input to the swap function a struct and refactor tests Dec 19, 2023
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (05eea03) 70.54% compared to head (1b5656c) 70.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2206      +/-   ##
==========================================
- Coverage   70.54%   70.47%   -0.07%     
==========================================
  Files         291      291              
  Lines       22672    22629      -43     
==========================================
- Hits        15993    15947      -46     
- Misses       5807     5809       +2     
- Partials      872      873       +1     
Files Coverage Δ
precompiles/outposts/osmosis/osmosis.go 34.92% <100.00%> (-6.51%) ⬇️
precompiles/outposts/osmosis/tx.go 83.92% <100.00%> (ø)
x/evm/keeper/precompiles.go 82.11% <100.00%> (-0.97%) ⬇️
precompiles/outposts/osmosis/types.go 82.96% <63.63%> (-4.85%) ⬇️

@0xstepit
Copy link
Contributor

I like the approach of reducing the number of inputs in functions but to be honest I don't see any advantage in doing it in this case. We should avoid >2/3 inputs in internal functions because it will make maintainability easier and the code cleaner. But, what is the purpose of using a struct to group all inputs from the user API like a contract? The user should pass the same number of params AND group them in a struct.

@Vvaradinov
Copy link
Contributor Author

@0xstepit I was researching this and the recommendation I am making is to keep it a struct due to Solidity structs being packed thus providing more efficiency for direct calls which is what I believe will the majority of calls for the Outpost. reference

@Vvaradinov Vvaradinov marked this pull request as ready for review December 20, 2023 14:46
@Vvaradinov Vvaradinov requested a review from a team as a code owner December 20, 2023 14:46
@Vvaradinov Vvaradinov requested review from facs95, GAtom22, MalteHerrmann and 0xstepit and removed request for a team December 20, 2023 14:46
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.

we'll need to remove the wevmos logic from the nix tests here to avoid getting error. Similarly to how we did it on the release/v16 branch. The thing is that we removed the registerCoin proposal from main. That's what the error is about.

I guess we'll need a wevmos address from genesis according to the logic we have now.

@Vvaradinov
Copy link
Contributor Author

@GAtom22 since we were merging to the release branch directly the logic is not here, but I think we should merge back and make the main branch up to date before merging it to the release branch.

Copy link

linear bot commented Jan 2, 2024

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 @Vvaradinov!!

@Vvaradinov Vvaradinov merged commit 3bcd7ab into main Jan 2, 2024
39 of 41 checks passed
@Vvaradinov Vvaradinov deleted the Vvaradinov/osmosis-outpost-refactor branch January 2, 2024 13:52
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