Gabe's review

[grod220]

Testing 🧪

• Here’s a peek at your code coverage. I’d encourage you to run them as well (I use CLion’s coverage abilities) and find the holes. For example: Entry point queries could use some tests.
  • 
• I think the nature of this application really requires something that can test contract-to-contract functionality. Perhaps that’s why there isn’t coverage for lots of messages? Some things to consider:
  • I have used cw-multi-test extensively and it has worked work well. However, it may require you to mock out some contracts or chain modules to be able to complete full contract→contract→chain module requests.
  • For Osmosis, we’ve also used Test-Tube which is the most accurate to the chain (but does not have good debugging).
• If there is only two cases tested (a happy and bad), I’d say testcase is not necessary.
  • In general, would you say testcase is helpful? Personally, I have used it sparingly. For this repo, I find it makes it quite difficult to reason about these cases. And given it’s in a macro, the IDE kinda treats it as plain text and hard to read.
• I think you should be asserting here the changed state of the bank balances of the contracts and the user.

Frontend scripts 🐍

Your deploy scripts are not type safe and are written in Python. I think you should convert this to Typescript (the sooner the better). Few things:

• You want to tightly couple your contract messages with your deploy scripts. At the moment, these are entirely separate. This means the contracts can break the deploy scripts and you wouldn’t know.
• The frontend team already needs to write Typescript and convert these Rust messages to Typescript types. Your deploy scripts could partially serve as a blueprint for them. Further, you should be using ts-codegen to generate the types the frontend needs to use. Doing so, will tightly couple their types with the contracts. So much safer!
• Add yarn build checks and ts-codegen generation to your CI/CD so it ensures your contracts never unexpectedly break frontend types.
• Consider adding simple user flows to your deploy scripts that can run in testnet
• Your deploy scripts should also automatically generate these files from the result of their running
• Good examples here. If you are interested in all of this I could walk you through how this works too.

Chain Abstraction ⛓️

• Your thoughts are definitely valid here. This is a great time to start abstracting things out. We ran into this exact issue and found this pattern to be effective. You define a base struct that takes generics which implement general functions. It’s quite hard to fully explain and is much easier to straight copy this model from Mars.

Random observations ☕️

• I see you are using a Makefile, but we’ve been quite happy using cargo make. Here’s our Makefile.toml.
• Your build cache looks to be using an old version. The new way should look like: --mount type=volume,source="$(basename "$(pwd)")_cache",target=/target \
• Some of your deps are out of date. I encourage you to run cargo upgrade (in cargo-edit package) and run it often.
• You should add a cargo fmt check to your ci/cd pipeline to ensure folks are formatting their code.
• Your github workflow only runs on PR. I’d recommend to add main as well. You always want to know if your main builds after a merge. There is a situation where someone merges an old PR that had passed all CI/CD checks, but breaks main. It’s good to be aware of that after merge and not the next time someone creates a new PR.
• Instead of a path dependency, make this skip = { workspace = true } and add the dependency to the top level workspace Cargo.toml like so: skip = { version = "2.0.0", path = "./packages/skip" }. That way, it makes it easier for contracts to inherit.
• The instantiate function has a lot of validation logic inside. I’d encourage you to abstract this. There is a pattern you could use of having messages that take unchecked versions and have a unchecked_swap_venue.checked() method called in instantiate. That struct can also have other logic on it (e.g. how to assemble swap messages).
• You probably want a set here. Larry created a good crate for that purpose.
• Why is it called a Venue and not a Dex?
• Consider adding code comments for documentation on your execute messages. Not sure what an Affiliate is.
• If fee_swap is only for IBC transfers, you should place it within the action variant instead of making it an option.
• How can you determine the refund amount at this stage? Isn’t it just an estimate? Don’t you need to issue the swap first, see how much is left over, and then issue the refund? For this, you’d need to have a callback message with the current balance and check the balance after the action has taken place. Or perhaps I’m not understanding this precisely right.
• This file is getting a bit long. Could be a candidate for splitting up functions into separate files. This is also a personal preference. I tend to err on splitting up code earlier rather than later.
• If you don’t set the contract version, you won’t be able to upgrade the contract.
• You should have an owner role that is allowed to update the config and state of the contract. Else, those values become immutable. I wrote this one and it’s in use with all Mars contracts. Examples: initialize / update / use in execute
• Doing it this way means you can only ever have one reply message. Would encourage you to do a match and throw an error vs panic with unreachable!().

[NotJeremyLiu]

• Here’s a peek at your code coverage. I’d encourage you to run them as well (I use CLion’s coverage abilities) and find the holes. For example: Entry point queries could use some tests.

  • 

ack, will try out CLion and see what's missing, all the ExecuteMsg's are tested but many of the helpers are indirectly tested through the ExecuteMsg tests, interested to see how CLion interprets this

[NotJeremyLiu]

• I think the nature of this application really requires something that can test contract-to-contract functionality. Perhaps that’s why there isn’t coverage for lots of messages? Some things to consider:

  • I have used cw-multi-test extensively and it has worked work well. However, it may require you to mock out some contracts or chain modules to be able to complete full contract→contract→chain module requests.
  • For Osmosis, we’ve also used Test-Tube which is the most accurate to the chain (but does not have good debugging).

@grod220 Can you point me to a repo that mocks chain modules? When I first started working on the contracts, we were using cw-multi-test which lacked stargate messages and other chain modules (needed for Neutron transfer module). Also at that time, osmosis test-tube did not support the poolmanager module, so it seemed integration tests became more of a hassle than helpful.

Currently, all cross-contract testing is happening on testnets, but I agree this is not ideal and I'd love a smooth local cross-contract testing flow.

[NotJeremyLiu]

If there is only two cases tested (a happy and bad), I’d say testcase is not necessary. In general, would you say testcase is helpful? Personally, I have used it sparingly. For this repo, I find it makes it quite difficult to reason about these cases. And given it’s in a macro, the IDE kinda treats it as plain text and hard to read.

This may be just a personal preference after working in the Osmosis repo (they do similar things in their test cases where they have cases and a single function to handle the tests), but I am a fan of the test case structure.

Agreed overkill when only two test cases but was just set up that way to keep a standard across all test files.

[NotJeremyLiu]

I think you should be asserting here the changed state of the bank balances of the contracts and the user.

These tests only output messages, none of the messages are ran (so bank balances are never changed, just bank sends are emitted). Would need to move to cw-multi-test to get state changes, but still am looking for a better way to use cw-multi-test with stargate and custom modules.

[NotJeremyLiu]

Your deploy scripts are not type safe and are written in Python. I think you should convert this to Typescript (the sooner the better). Few things: The frontend team already needs to write Typescript and convert these Rust messages to Typescript types. Your deploy scripts could partially serve as a blueprint for them. Further, you should be using ts-codegen to generate the types the frontend needs to use. Doing so, will tightly couple their types with the contracts. So much safer! Add yarn build checks and ts-codegen generation to your CI/CD so it ensures your contracts never unexpectedly break frontend types.

I am seemingly on the wrong side of history, I write all my scripts in python, will explore this.

@grod220 For our contracts in particular, FE typescript devs do not interact with our contracts, the messages are all generated by our server in Go (our users just use our API which gives them a message that they then send to a wallet to sign). With that understanding, would you recommend making the deploy scripts in Go since the main benefit you seem to be highlighting is type safety and example providing for the main service that interacts with the contracts?

[NotJeremyLiu]

Consider adding simple user flows to your deploy scripts that can run in testnet

Ack, I have a .ipynb (more python) that I do execution of user flows (many of em, and was helpful to execute 1 by 1 and see results), but makes sense to couple more tightly with the main automated script.

[NotJeremyLiu]

Your deploy scripts should also automatically generate these files from the result of their running

They actually do this today! The deploy script generates all the files in deployed-contracts

[NotJeremyLiu]

https://github.com/skip-mev/skip-api-contracts/issues/54. This is a great time to start abstracting things out. We ran into this exact issue and found this pattern to be effective. You define a base struct that takes generics which implement general functions. It’s quite hard to fully explain and is much easier to straight copy this model from Mars.

Ack, thank you! Will continue to work though this design!

[NotJeremyLiu]

I see you are using a Makefile, but we’ve been quite happy using cargo make. Here’s our Makefile.toml.

ack, Larry also had a suggestion to use justfile, but we're currently using Makefile since it's a company standard at Skip across all our codebases

[NotJeremyLiu]

Your build cache looks to be using an old version. The new way should look like: --mount type=volume,source="$(basename "$(pwd)")_cache",target=/target \

This has no material impact right? This is just changing where the cache is stored? being in the makefile will make this command be changed anyways since pwd can't be used directly (will be shell pwd) and will also need to do notdir to make the pathing valid, so prefer to keep as is if no impact.

Will change the target to /target as that seems to be the new standard defined in the changelog: https://github.com/CosmWasm/rust-optimizer/blob/main/CHANGELOG.md

Done here: https://github.com/skip-mev/skip-api-contracts/pull/58

[NotJeremyLiu]

Some of your deps are out of date. I encourage you to run cargo upgrade (in cargo-edit package) and run it often.

Sweet, done here: https://github.com/skip-mev/skip-api-contracts/pull/57

[NotJeremyLiu]

You should add a cargo fmt check to your ci/cd pipeline to ensure folks are formatting their code.

Makes sense, added here: https://github.com/skip-mev/skip-api-contracts/pull/59

[NotJeremyLiu]

Your github workflow only runs on PR. I’d recommend to add main as well. You always want to know if your main builds after a merge. There is a situation where someone merges an old PR that had passed all CI/CD checks, but breaks main. It’s good to be aware of that after merge and not the next time someone creates a new PR.

Good callout, updated here: https://github.com/skip-mev/skip-api-contracts/pull/60

[NotJeremyLiu]

Instead of a path dependency, make this skip = { workspace = true } and add the dependency to the top level workspace Cargo.toml like so: skip = { version = "2.0.0", path = "./packages/skip" }. That way, it makes it easier for contracts to inherit.

This is sweet, thanks, done: https://github.com/skip-mev/skip-api-contracts/pull/61

[NotJeremyLiu]

Why is it called a Venue and not a Dex?

This was just a naming we decided on and share across the contracts and our api service, no real reason beyond that

[NotJeremyLiu]

How can you determine the refund amount at this stage? Isn’t it just an estimate? Don’t you need to issue the swap first, see how much is left over, and then issue the refund? For this, you’d need to have a callback message with the current balance and check the balance after the action has taken place. Or perhaps I’m not understanding this precisely right.

@grod220 The query simulates the exact swap that is about to take place using the method provided by the dex (Osmosis poolmanager module, and Astroport router/pools), and there is no state transition between the query and the swap itself so the output of the query would align with the swap itself. That is, if I want 10 osmo out and the query returns that it requires 5 atom in, then I can swap in 5 atom and get 10 osmo.

This can be an issue if the Osmosis poolmanager, Astroport router, or other dexes we add support for do not properly implement their query methods and they diverge from the actual swaps themselves, but under the assumption of correct query methods from the dex itself this should be fine.

Is your understanding different or is the above issue what flagged you?

[NotJeremyLiu]

Doing it this way means you can only ever have one reply message. Would encourage you to do a match and throw an error vs panic with unreachable!().

Just for my understanding, the match implementation you linked to does the exact same thing as our implementation currently, correct (it only supports a single reply id that's a hardcoded constant)?

But you're advocating that if we switch to the match pattern now, then if we ever wanted to add another submsg reply in the future it would be easier to implement? Or are there benefits of this approach even today with a single reply id?

[NotJeremyLiu]

If you don’t set the contract version, you won’t be able to upgrade the contract. You should have an owner role that is allowed to update the config and state of the contract. Else, those values become immutable. I wrote this one and it’s in use with all Mars contracts. Examples: initialize / update / use in execute

Ack, we decided against any owner/admin/upgradeability as we are unclear on the legal implications of that as a U.S. company. I think it is likely we do not implement these for those reasons.

[NotJeremyLiu]

If fee_swap is only for IBC transfers, you should place it within the action variant instead of making it an option.

This makes sense, realized this as I was working on v0.2.0 but didn't want to make another interface breaking change, but it probably is worthwhile to do so, so will refactor it.

Done here: https://github.com/skip-mev/skip-api-contracts/pull/63

[NotJeremyLiu]

You probably want a set here. Larry created a good crate for that purpose.

Ack, a little hesitant on adding a third-party dependency where a future maintainer may have to understand the source code instead of an easy to reason about map usage, but will explore

[NotJeremyLiu]

The instantiate function has a lot of validation logic inside. I’d encourage you to abstract this. There is a pattern you could use of having messages that take unchecked versions and have a unchecked_swap_venue.checked() method called in instantiate. That struct can also have other logic on it (e.g. how to assemble swap messages).

I think this makes sense to me, is this what you had in mind?: https://github.com/skip-mev/skip-api-contracts/pull/62

[grod220]

Can you point me to a repo that mocks chain modules?

I believe ApolloDAO was doing this here: https://github.com/apollodao/cw-it/blob/master/src/multi_test/modules/token_factory.rs. There may be better examples in the wild though 🤔

the messages are all generated by our server in Go (our users just use our API which gives them a message that they then send to a wallet to sign). With that understanding, would you recommend making the deploy scripts in Go

You need a way to tightly couple the Rust message enums/types with your A) deploy scripts and B) language of consumers. For A, I'd say ts-codegen + typescript does this well. For B, how are you doing this now? Do you have a package that can automatically generate those types? If not, I think it's a worthwhile venture figuring that out. Maybe something like this?. Also, it doesn't look like you are generating the schema for your contracts. See v2-fields for a good example of how we do this. See the examples folder in use for each contract.

This has no material impact right? This is just changing where the cache is stored?

Yes that's right

The query simulates the exact swap that is about to take place

I don't know if that can be trusted or that your contract should be relying on what they say will happen. I think the contract needs to verify this itself. If not, it may expose a vulnerability to the services you integrate with. Those simulation endpoints are more for frontend querying I'd say.

Just for my understanding, the match implementation you linked to does the exact same thing as our implementation currently, correct (it only supports a single reply id that's a hardcoded constant)?

Yes, that is correct. It's just like the way that execute or query is handled. Potentially, you could have more than one and therefore that function should just be concerned with routing and not business logic.

we decided against any owner/admin/upgradeability

Whoa, bold move 😅 . Why not start with an owner and then execute the message that abolishes the role (makes it immutable) later?

I think this makes sense to me, is this what you had in mind?: https://github.com/skip-mev/skip-api-contracts/pull/62

Yes, that's a step in the direction. However, I'd say that you should have a UncheckedSwapVenue that has the validate message that returns a SwapVenue and then your SWAP_VENUE_MAP should hold SwapVenue and not address.