Skip to content

simplify mars_outpost::red_bank exports#33

Merged
larry0x merged 6 commits intomasterfrom
larry/red-bank-improvements
Aug 11, 2022
Merged

simplify mars_outpost::red_bank exports#33
larry0x merged 6 commits intomasterfrom
larry/red-bank-improvements

Conversation

@larry0x
Copy link
Contributor

@larry0x larry0x commented Aug 11, 2022

currently the way the mars_outpost::red_bank module works is, there are some stuff defined in mod.rs, some defined in interest_rate_model.rs, some in msg.rs, and i need to know where each thing is, and import them separately from each sub-module:

use mars_outpost::red_bank::FirstThing;
use mars_outpost::red_bank::interest_rate_model::SecondThing;
use mars_outpost::red_bank::msg::ThirdThing;

this doesn't make sense to me... this PR:

  • moves types defined in mod.rs to a separate types.rs file
  • export types defined in sub-modules, so that all things Red Bank can be imported in a single place:
use mars_outpost::red_bank::{FirstThing, SecondThing, ThirdThing};

dancreee
dancreee previously approved these changes Aug 11, 2022
Copy link

@dancreee dancreee left a comment

Choose a reason for hiding this comment

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

This looks like an improvement to me - however with anything somewhat subjective I think we should

a. determine if a refactor introduces any additional bug/exploit risk
b. have all core devs align on this as a coding standard and put it in the guidelines - with the goal to remove most subjective changes from day to day PR's

@piobab thoughts on this and adding it to the guidelines

@piobab
Copy link
Collaborator

piobab commented Aug 11, 2022

This looks like an improvement to me - however with anything somewhat subjective I think we should

a. determine if a refactor introduces any additional bug/exploit risk b. have all core devs align on this as a coding standard and put it in the guidelines - with the goal to remove most subjective changes from day to day PR's

@piobab thoughts on this and adding it to the guidelines

"moves types defined in mod.rs to a separate types.rs file" - OK

When it comes to re-exports, we have to remember:
Putting everything in one bag can be convenient for importing, but it removes modules with the right logic, the right types (we can't do module::function). Additionally, if the same structure or function name is used in another module, it will result in a name conflict.
For sure it will help to use by external clients (contracts).

@grod220
Copy link
Contributor

grod220 commented Aug 11, 2022

I don't have a strong opinion on this. Intelliji automatically finds/imports and it also hides imports by default in every file. So the way they look is almost irrelevant to my dev workflow. My only concern would be to not make the types.rs file a monolith if they could live closer to their usage.

@larry0x larry0x changed the base branch from larry/break-down-big-files to master August 11, 2022 10:41
@larry0x larry0x dismissed dancreee’s stale review August 11, 2022 10:41

The base branch was changed.

Copy link
Contributor

@ahmadkaouk ahmadkaouk left a comment

Choose a reason for hiding this comment

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

For me, I like to have smaller import sections and shorter paths. But this personal preference.

piobab
piobab previously approved these changes Aug 11, 2022
@larry0x
Copy link
Contributor Author

larry0x commented Aug 11, 2022

@dancreee @piobab can you approve again? i had to resolve the conflicts after merging #31, which dismissed your previous approvals

Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

I'll approve this (as I hate automatic PR approval dismissals lol)

@larry0x larry0x merged commit b022c00 into master Aug 11, 2022
@larry0x larry0x deleted the larry/red-bank-improvements branch August 11, 2022 11:22
@dancreee
Copy link

I'll approve this (as I hate automatic PR approval dismissals lol)

ok the crowd has spoken, i'll remove this

larry0x pushed a commit that referenced this pull request Oct 7, 2023
* integrate cw-set for allowed coins

* review updates
markonmars added a commit that referenced this pull request Nov 3, 2025
* add swap fee

* fmt

* fix tsc

* improve assert_swap_fee
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.

5 participants