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

Stream Payment pallet #391

Merged
merged 44 commits into from
Feb 16, 2024
Merged

Stream Payment pallet #391

merged 44 commits into from
Feb 16, 2024

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Jan 18, 2024

Copy link
Contributor

github-actions bot commented Jan 18, 2024

Coverage Report

(master)

@@                       Coverage Diff                        @@
##           master   jeremy-stream-payment-pallet      +/-   ##
================================================================
- Coverage   76.66%                         76.30%   -0.36%     
+ Files         107                            109       +2     
+ Lines       26803                          27541     +738     
================================================================
+ Hits        20547                          21014     +467     
+ Misses       6256                           6527     +271     
Files Changed Coverage
/runtime/dancebox/src/lib.rs 79.13% (-1.01%) 🔽
/runtime/dancebox/tests/integration_test.rs 99.73% (+0.01%) 🔼
/runtime/flashbox/src/lib.rs 33.61% (-2.64%) 🔽

Coverage generated Fri Feb 16 10:36:49 UTC 2024

/// this AccountId is a target in StreamId. One can iterate over all storage
/// keys starting with the AccountId to find all StreamIds.
#[pallet::storage]
pub type LookupStreamsWithTarget<T: Config> = StorageDoubleMap<
Copy link
Contributor

Choose a reason for hiding this comment

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

These double maps are not used by the pallet, it would probably be better to remove them and calculate them offchain. Unless they should be used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are here for the UI to find the list of streams one is the source/target.

@girazoki
Copy link
Collaborator

I think it looks good overall, just noted down a couple of observations. Also let's try to configure this in Dancebox and Flashbox, and add some integration tests to it

Comment on lines +925 to +926
fn source_can_immediately_decrease_deposit() {
source_can_immediately_change_deposit(DepositChange::Decrease(100))
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this defeat the purpose of a deposit? I get that the source can already close the stream at any time and stop the payment, but this feels strange to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ok, sometimes source might have put too much in the deposit. The target as well can close the stream if the amount is not being paid, so I think it's ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed the idea. Since the source can close the stream at any time and recover unspent deposit, I think we can allow to decrease the deposit without closing the stream. One could first deposit a big amount because they want a long-term service, but at some point temporarily need some funds for something else. What matters for the target is that past time is guaranteed to be paid, and by looking at the events they can compute up to which time the stream is funded.

@nanocryk nanocryk added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Feb 9, 2024
@nanocryk nanocryk added the not-breaking Does not need to be mentioned in breaking changes label Feb 9, 2024
@nanocryk nanocryk merged commit 90433bb into master Feb 16, 2024
31 checks passed
@nanocryk nanocryk deleted the jeremy-stream-payment-pallet branch February 16, 2024 14:46
@girazoki girazoki added the D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants