Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse Splitter service #49

Merged
merged 26 commits into from
Oct 8, 2024
Merged

Reverse Splitter service #49

merged 26 commits into from
Oct 8, 2024

Conversation

stiiifff
Copy link
Collaborator

@stiiifff stiiifff commented Oct 3, 2024

Implemented:

  • Implementation according to spec
  • Support for fixed amounts or ratios
  • Supports native & CW20 tokens
  • Extensive tests

Todo:

  • Dynamic ratio support via an external contract
  •  Update ServiceConfig to use ServiceAccountType for input & output accounts

Copy link
Contributor

@Art3miX Art3miX left a comment

Choose a reason for hiding this comment

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

Waiting for the enum change

Cargo.toml Outdated
Comment on lines 56 to 58
valence-splitter-service = { path = "contracts/services/splitter", features = ["library"] }
valence-test-dynamic-ratio = { path = "contracts/testing/test-dynamic-ratio", features = ["library"] }
valence-test-service = { path = "contracts/testing/test-service", features = ["library"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 2 to 4
name = "valence-reverse-splitter-service"
authors = { workspace = true }
edition = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

}
}

// // Prepare transfer messages for each denom
Copy link
Contributor

Choose a reason for hiding this comment

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

//

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

contracts/services/reverse-splitter/README.md Show resolved Hide resolved
@keyleu keyleu self-requested a review October 7, 2024 21:29
Comment on lines +245 to +250
let res: DynamicRatioResponse = querier.query_wasm_smart(
contract_addr,
&DynamicRatioQueryMsg::DynamicRatio {
denoms: vec![denom_name.clone()],
params: params.to_string(),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding querying a dynamic ration, we would probably need a wrapper of our own that will support this query because 99% other contracts wont.
Just something to note somewhere so we won't forgot.

Comment on lines 173 to 176
pub fn with_factor(mut self, factor: u64) -> Self {
self.factor = Some(factor);
self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the helper "with_" function are creating a new struct, I would suggest calling this as "set_factor" instead of with to make it clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

Comment on lines 390 to 395
if splits
.iter()
.any(|s| !matches!(s.amount, UncheckedSplitAmount::FixedAmount(..)))
&& splits.iter().any(|s| {
matches!(s.amount, UncheckedSplitAmount::FixedAmount(..)) && s.denom != *base_denom
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We already doing a loop above, we can have a flag set to true at the above loop if any !UncheckedSplitAmount::FixedAmount(..), instead of doing another loop here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 34 to 39
cw20 = { workspace = true }
cw20-base = { workspace = true }
sha2 = { workspace = true }
valence-account-utils = { workspace = true }
valence-service-utils = { workspace = true, features = ["testing"] }
valence-test-dynamic-ratio = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

// Instantiate Splitter contract
suite.splitter_init(&cfg);
}

#[test]
#[should_panic(
expected = "Configuration error: Duplicate split 'Native(\"untrn\")|AccountAddr(\"cosmwasm1ea6n0jqm0hj663khx7a5xklsmjgrazjp9vjeewejn84sanr0wgxq2p70xl\")' in split config."
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this line because I renamed an enum variant from AccountAddr to Addr,
Would be great if we can avoid those should_panic when we need to include some parsing of enums or alike.

Comment on lines +29 to +31
#[getset(get)]
inner: ServiceTestSuiteBase,
#[getset(get)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of getset(get) again? you are building the get functions below anyways

);
}

// // Native & CW20 token amount splits
Copy link
Contributor

Choose a reason for hiding this comment

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

double //

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@stiiifff stiiifff merged commit c3822f2 into main Oct 8, 2024
1 check passed
@stiiifff stiiifff deleted the sde/reverse-splitter-service branch October 8, 2024 11:44
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.

3 participants