Skip to content

Conversation

ahmadkaouk
Copy link
Contributor

  • Remove the contracts that won't be deployed on outposts. (mars-council, mars-red-bank-closure, mars-staking, mars-treasury, mars-vesting, mars-xmars-token)
  • Update build settings
  • Remove Astroport, Stader and Lido dependencies and related code.
  • Disable the compilation of the smart contracts temporarily.

@ahmadkaouk ahmadkaouk changed the title Remove contacts not getting deployed on outposts and some code cleanup Remove contracts not getting deployed on outposts and some code cleanup Jun 28, 2022
authors = ["Spike Spiegel <spikeonmars@protonmail.com>"]
name = "mars-outpost-core"
version = "0.1.0"
authors = ["Ahmad Kaouk <ahmad@delphilabs.io"]
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 we should include all authors who have contributed to the code base:

authors = [
  "Ahmad Kaouk <ahmad@delphilabs.io",
  "Spike Spiegel <spikeonmars@protonmail.com>",
  "larry_0x <larry@delphidigital.io>",
  "Piotr Babel",
  "Harry Scholes"
]

@larry0x
Copy link
Contributor

larry0x commented Jun 29, 2022

@ahmadkaouk added comments regarding Cargo.toml but otherwise lgtm

@kribby kribby requested a review from SunshineRider June 29, 2022 10:14
@grod220
Copy link
Contributor

grod220 commented Jun 29, 2022

I wonder if it would be easier to start from a blank slate and adding in features one-by-one (with passing builds). I'm doing this with fields. Otherwise, you may not get a passing build for a looooong time.

pub enum PriceSource<A> {
/// Returns a fixed value; used for UST
pub enum PriceSource {
/// Returns a fixed value; used for OSMO

Choose a reason for hiding this comment

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

Don't think we should have any chain specific tokens mentioned in core codebase (I know I put it there! :P )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I will remove this in the next PR.

/// Amount to distribute to protocol contracts, defaults to contract balance if not specified
amount: Option<Uint128>,
},

Choose a reason for hiding this comment

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

while we can remove this swap for now - we'll have to still have some swap logic in here - we need to to probably have this swap asset set as a config item on the protocol_rewards_collector contract, so anyone can still initiate the message but it's the config that stores what asset it will be swapped into (as each change might/will be different)

Choose a reason for hiding this comment

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

We will have a story around the protocol rewards collector refactor where we can add this back in

@dancreee
Copy link

I wonder if it would be easier to start from a blank slate and adding in features one-by-one (with passing builds). I'm doing this with fields. Otherwise, you may not get a passing build for a looooong time.

Yea, this is a tough one...

What this way gives us, is a way to easily diff the changes from terra -> osmosis, so it's easy to see what has changed... however it's also very message with just deleting a bunch of stuff etc and there as so many intertwined dependencies that it's hard to migrate each contract across independently...

On the other hand, starting from scratch will take longer and will mean we have to audit/review more code overall, i'm on the fence as to which one is better - we know it can be more or less migrated as we have a PoC doing so already

@ahmadkaouk
Copy link
Contributor Author

I wonder if it would be easier to start from a blank slate and adding in features one-by-one (with passing builds). I'm doing this with fields. Otherwise, you may not get a passing build for a looooong time.

Yea, this is a tough one...

What this way gives us, is a way to easily diff the changes from terra -> osmosis, so it's easy to see what has changed... however it's also very message with just deleting a bunch of stuff etc and there as so many intertwined dependencies that it's hard to migrate each contract across independently...

On the other hand, starting from scratch will take longer and will mean we have to audit/review more code overall, i'm on the fence as to which one is better - we know it can be more or less migrated as we have a PoC doing so already

Yea, so as @dancreee mentioned, this way make it easier to track changes (diff from the original mars-core repo). To overcome the problem of builds, I disabled the build of the smart contracts temporarily, so I could still be able to test for a passing build and run unit tests after each change. I'll progressively re-enable the build of each contract after finishing its migration.

@ahmadkaouk ahmadkaouk requested a review from larry0x June 30, 2022 08:24
@ahmadkaouk ahmadkaouk merged commit 632a663 into master Jul 4, 2022
@larry0x larry0x deleted the code-cleanup branch August 4, 2022 17:25
@dancreee dancreee mentioned this pull request Aug 9, 2022
larry0x pushed a commit that referenced this pull request Oct 7, 2023
Adding allow-list for vaults and assets
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.

4 participants