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

feat: support for opening/closing private broker channels #5361

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Closes: PRO-1748

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.

Summary

  • Added a storage map AccountId -> ChannelId
  • Added Extrinsics to open/close private channels + tests + benchmarks

Out of scope (these seem like may need some further discussion):

  • Charging fee/rent for opening a private channel/keeping it open
  • Closing private channels upon account deletion (maybe this isn't necessary for an MVP?)
    (will create separate linear tickets for these if we agree to leave them out of this PR)

May need to rebase this after #5357 is merged.

@@ -582,6 +582,10 @@ pub mod pallet {
pub(crate) type ScheduledTxForReject<T: Config<I>, I: 'static = ()> =
StorageValue<_, Vec<TaintedTransactionDetails<T, I>>, ValueQuery>;

#[pallet::storage]
pub(crate) type BrokerPrivateChannels<T: Config<I>, I: 'static = ()> =
StorageMap<_, Twox64Concat, T::AccountId, ChannelId, OptionQuery>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can use the Identity hasher.

Suggested change
StorageMap<_, Twox64Concat, T::AccountId, ChannelId, OptionQuery>;
StorageMap<_, Identity, T::AccountId, ChannelId, OptionQuery>;

@msgmaxim msgmaxim force-pushed the chore/rename-btc-smart-contract branch from d59a952 to d407b8c Compare October 30, 2024 01:01
Base automatically changed from chore/rename-btc-smart-contract to main October 30, 2024 02:23
Comment on lines 1341 to 1345
let next_channel_id =
ChannelIdCounter::<T, I>::try_mutate::<_, Error<T, I>, _>(|id| {
*id = id.checked_add(1).ok_or(Error::<T, I>::ChannelIdsExhausted)?;
Ok(*id)
})?;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make this a function, is duplicate

let broker_id = T::AccountRoleRegistry::ensure_broker(origin)?;

ensure!(
!BrokerPrivateChannels::<T, I>::contains_key(&broker_id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also restrict this to be just for Bitcoin - it's not useful for any other chains right? probably more importantly is that not restricting it to Bitcoin would be confusing when reading the code, since it's an instantiable pallet, we'd assume it's for all instances.

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
@dandanlen
Copy link
Collaborator

Closing private channels upon account deletion (maybe this isn't necessary for an MVP?)
(will create separate linear tickets for these if we agree to leave them out of this PR)

I feel like we can just add this here. We need it and it's not a lot of work to add. Not sure what we gain by waiting.

(Sorry was sure I left this comment yesterday...)

@dandanlen
Copy link
Collaborator

Forgot to mention - I think charging/not charging a fee needs a little more discussion, happy to leave this out for now.

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