feat(interchain-token-service): add flow limit #130
Conversation
- implement flow limit for interchain transfers - add Operatable trait and change constructor - update golden files (tests generate different contract addresses due to updated constructor) - minor changes to test helper functions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 94.63% 94.84% +0.20%
==========================================
Files 52 53 +1
Lines 3130 3276 +146
==========================================
+ Hits 2962 3107 +145
- Misses 168 169 +1 ☔ View full report in Codecov by Sentry. |
- Add docstrings to entrypoints - Create FlowKey struct for better organization of storage keys - Move flow logic into FlowDirection impl - Organize imports and group public functions - Allow zero flow limit to 'freeze' token
| pub fn set_flow_limit( | ||
| env: &Env, | ||
| token_id: BytesN<32>, | ||
| flow_limit: Option<i128>, |
There was a problem hiding this comment.
why not make this u128 to avoid the ensure below?
There was a problem hiding this comment.
It's convention on Stellar to use i128 for token amounts. It's because of unrelated operations (clawbacks, etc), but we leave it as i128 for consistency
|
|
||
| direction.update_flow(env, token_id.clone(), new_flow); | ||
|
|
||
| extend_persistent_ttl(env, &DataKey::FlowLimit(token_id)); |
There was a problem hiding this comment.
update_flow uses the DataKey::FlowIn(FlowKey) key yet you extend the ttl (which I'm assuming is the lifetime) of the DataKey::FlowLimit(BytesN<32>) key. Why?
There was a problem hiding this comment.
To me it seems to be the best place to extend the ttl as it's the only write operation that regularly touches the flow limit. Do you see a better place for it?
| .ok_or(ContractError::FlowLimitExceeded)?; | ||
| let max_allowed = flow_to_compare | ||
| .checked_add(flow_limit) | ||
| .ok_or(ContractError::FlowLimitExceeded)?; |
There was a problem hiding this comment.
If you decide to keep i128 instead of u128 then you can subtract flow_limit (which is guaranteed to be positive but less than i128::MAX) from new_flow so that you can never overflow here.
There was a problem hiding this comment.
I was thinking about this but we are already planning to introduce a custom Uint127 type which I think will be easier to reason about
| flow_limit::flow_out_amount(env, token_id) | ||
| } | ||
|
|
||
| /// Retrieves the flow out amount for the current epoch for the token |
There was a problem hiding this comment.
| /// Retrieves the flow out amount for the current epoch for the token | |
| /// Retrieves the flow in amount for the current epoch for the token |
| fn flow_limit(env: &Env, token_id: BytesN<32>) -> Option<i128> { | ||
| flow_limit::flow_limit(env, token_id) | ||
| } | ||
|
|
||
| /// Retrieves the flow out amount for the current epoch for the token | ||
| /// associated with the specified token ID. |
There was a problem hiding this comment.
explain what flow out means (might be interpreted as either of the directions)
| @@ -162,10 +162,42 @@ impl InterchainTokenServiceInterface for InterchainTokenService { | |||
| .into() | |||
| } | |||
|
|
|||
| /// Retrieves the flow limit for the token associated with the specified token ID. | |||
There was a problem hiding this comment.
Add the doc in the interface.rs instead. The contract.rs could have a note like to link it
/// See [`InterchainTokenServiceInterface::xyz`]
| /// # Errors | ||
| /// - `ContractError::InvalidFlowLimit`: If the provided flow limit is not positive. | ||
| /// | ||
| /// Authorization: Only the operator can call this function. Unauthorized calls will panic. |
There was a problem hiding this comment.
| /// Authorization: Only the operator can call this function. Unauthorized calls will panic. | |
| /// # Authorization | |
| /// - Must be called by the [`Self::operator`]. |
perhaps?
| @@ -16,6 +16,7 @@ pub struct TrustedChainRemovedEvent { | |||
| #[derive(Debug, PartialEq, Eq)] | |||
| pub struct FlowLimitSetEvent { | |||
| pub token_id: BytesN<32>, | |||
| /// Setting to None bypasses flow limit checks | |||
There was a problem hiding this comment.
| /// Setting to None bypasses flow limit checks | |
| /// A `None` value implies that flow limit checks have been disabled for this `token_id` |
| /// Sets or updates the flow limit for a token. | ||
| /// | ||
| /// Flow limit controls how many tokens can flow in/out during a single epoch. | ||
| /// Setting the limit to None disables flow limit checks for the token. |
There was a problem hiding this comment.
| /// Setting the limit to None disables flow limit checks for the token. | |
| /// Setting the limit to `None` disables flow limit checks for the token. |
|
|
||
| #[cfg(any(test, feature = "testutils"))] | ||
| pub use testutils::*; | ||
|
|
||
| pub trait Event: Debug + PartialEq { | ||
| fn topics(&self, env: &Env) -> impl Topics + Debug; | ||
|
|
||
| fn data(&self, env: &Env) -> impl IntoVal<Env, Val> + Debug; | ||
| fn data(&self, env: &Env) -> impl IntoVal<Env, Val> + Debug { |
There was a problem hiding this comment.
| fn data(&self, env: &Env) -> impl IntoVal<Env, Val> + Debug { | |
| /// A default empty tuple/vector is used for event data, since majority of events only use topics. | |
| fn data(&self, env: &Env) -> impl IntoVal<Env, Val> + Debug { |
explain the reasoning
| enum FlowDirection { | ||
| In, | ||
| Out, | ||
| } |
There was a problem hiding this comment.
| enum FlowDirection { | |
| In, | |
| Out, | |
| } | |
| enum FlowDirection { | |
| /// An interchain transfer coming in to this chain from another chain | |
| In, | |
| /// An interchain transfer going out from this chain to another chain | |
| Out, | |
| } |
| enum FlowDirection { | ||
| In, | ||
| Out, | ||
| pub fn add_flow_in( |
There was a problem hiding this comment.
How about moving pub fn add_flow within FlowDirection, and then you can call FlowDirection::In.add_flow(...) from ITS contract?
AXE-6087