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]

#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: #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: #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: #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: #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: #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