Skip to content
This repository was archived by the owner on Oct 30, 2023. It is now read-only.

Conversation

CyberHoward
Copy link
Contributor

📢 Type of change

▫️ Bugfix
▫️ New feature
▫️ Enhancement
▫️ Refactoring
▫️ Maintenance

📜 Description and Motivation

🛠️ How to test (if applicable)

📝 Checklist

▫️ Reviewed submitted code
▫️ Added tests to verify changes
▫️ Updated docs
▫️ Verified on testnet

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #230 (d95b055) into main (22bc201) will increase coverage by 0.23%.
The diff coverage is 90.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
+ Coverage   53.16%   53.40%   +0.23%     
==========================================
  Files         114      116       +2     
  Lines        4117     4144      +27     
==========================================
+ Hits         2189     2213      +24     
- Misses       1928     1931       +3     
Impacted Files Coverage Δ
contracts/native/ibc-client/src/commands.rs 26.66% <ø> (ø)
packages/abstract-app/src/endpoints.rs 100.00% <ø> (ø)
packages/abstract-os/src/objects/pool/pool_id.rs 52.54% <ø> (ø)
packages/abstract-sdk/src/apis/verify.rs 81.25% <80.76%> (-9.08%) ⬇️
packages/abstract-os/src/modules/apis/dex.rs 100.00% <100.00%> (ø)
packages/abstract-os/src/objects/fee.rs 100.00% <100.00%> (ø)
packages/abstract-sdk/src/apis/dex.rs 73.17% <100.00%> (ø)
packages/abstract-sdk/src/cw_helpers/fees.rs 100.00% <100.00%> (ø)

@github-actions github-actions bot removed the api_base label Feb 15, 2023
@CyberHoward CyberHoward changed the title [draft] DEX usage fee DEX usage fee Feb 15, 2023
@CyberHoward CyberHoward requested a review from adairrr February 15, 2023 13:57
Comment on lines +40 to +47
#[cosmwasm_schema::cw_serde]
pub enum DexApiExecuteMsg {
Action(DexExecuteMsg),
UpdateFee {
swap_fee: Option<Decimal>,
recipient_os_id: Option<u32>,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So I guess the json would be:

{
  "app": {
    "action": {
      "action": {
        "swap": {...}
      },
      "dex": "lolswap",
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

{
  "app": {
    "execute": {
      "dex": "lolswap",
      "action": {
        "swap": {...}
      },
    }
  }
}

?

Copy link
Contributor

Choose a reason for hiding this comment

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

The users shouldn't be interacting with the json too much but still the nested duplicate names can be confusing.

Copy link
Contributor

@adairrr adairrr Feb 15, 2023

Choose a reason for hiding this comment

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

An alternative to this is to add a configure "endpoint" to the ApiExecuteMsg which would be a different "handler" on the API. That way we don't add additional nesting at the api module level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm.....
Executing:

{
  "app": {
    "dex": "lolswap",
    "action": {
      "swap": {...}
    },
  }
}

Configuring:

{
  "configure": {
    "update_fee": {...}
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to discuss this during standup

Copy link
Contributor Author

@CyberHoward CyberHoward Feb 15, 2023

Choose a reason for hiding this comment

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

I think adding a #[serde(flatten)] tag to the action variant should remove the nested nature of the msg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure if those unit-enum variants actually get their own key or if the inner value name is used as key.

Copy link
Contributor Author

@CyberHoward CyberHoward Feb 15, 2023

Choose a reason for hiding this comment

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

This code:

use serde::{Serialize, Deserialize};

#[derive(Serialize, Deserialize, Clone, Debug)]
struct FooFoo {
    x: i32,
}
#[derive(Serialize, Deserialize, Clone, Debug)]
enum Bar {
    Foo(FooFoo),
    Baz,
}

fn main() {
    let bar = Bar::Foo(FooFoo { x: 42 });

    println!("bar = {:?}", bar);
    println!("bar = {}", serde_json::to_string_pretty(&bar).unwrap());
}

Returns:

bar = Foo(FooFoo { x: 42 })
bar = {
  "Foo": {
    "x": 42
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also opt for a "type" tag which would look like:

bar = {
  "type": "Foo",
  "x": 42
}

And change the Action name to something different.

@CyberHoward CyberHoward merged commit 9998584 into main Feb 16, 2023
@CyberHoward CyberHoward deleted the feature/dex_fee branch February 16, 2023 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants