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

chore: add cap as storage service for claimable messages #62

Merged
merged 11 commits into from
Nov 8, 2022

Conversation

fcavazzoli
Copy link
Contributor

@fcavazzoli fcavazzoli commented Sep 20, 2022

Description

Changes:

  • Upgrade cap_cdk to 0.2.4
  • Store bridged assets (IC -> ETH) in CAP
  • delete candid method remove_claimable_asset since now they are stored in CAP
  • delete candid method get_claimable_messages because that query now needs to be perform to CAP root.
  • Add CAP-Route canister id as a constant in the proxies.
  • Use insert_sync method provided by cap_cdk to store the transactions
  • Update upgrade script to save all pending transactions in cap_cdk and now thats the only use for ProxyState.unclaimed_messages.


s.add_claimable_message(ClaimableMessage {
ic_kit::ic::spawn(async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a insert_sync which guarantees finality by doing the spawn for you

https://github.com/Psychedelic/cap/blob/main/sdk/rust/src/transaction/insert.rs#L126

}

pub async fn register_messages() {
let mut pending_registrations = STATE.with(|s| s.get_all_claimable_messages());
Copy link
Contributor

Choose a reason for hiding this comment

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

Then we can use the insert_many_sync which takes in a iterator of pending_registrations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theres no need for this now, every message is going to be processed sync and the pending ones are handled by cap_cdk.

STATE ClaimableAssets its only used during pre and post upgrade now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to my comment about flushing below

#62 (comment)

fn to_cap_event(&self) -> IndefiniteEvent;
}

impl ToEvent for ClaimableMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl ToEvent for ClaimableMessage {
impl ToCapEvent for ClaimableMessage {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, didnt commit this as commit suggestion because im importing in different files also, so I updated all at once.

});

// flush STATE claimable messages queue
register_messages().await
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, if we use cap insert sync it handles the flushing mechanism

Copy link
Contributor

@b0xtch b0xtch left a comment

Choose a reason for hiding this comment

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

LGTM, could we link an issue to this at least to explain why we are doing this

@fcavazzoli fcavazzoli marked this pull request as ready for review September 21, 2022 14:49
@fcavazzoli fcavazzoli linked an issue Sep 21, 2022 that may be closed by this pull request
2 tasks
@fcavazzoli fcavazzoli force-pushed the chore/use_cap_for_claimable_assets branch from 69c0b08 to 765207d Compare November 7, 2022 18:04
@fcavazzoli fcavazzoli merged commit 71cb0d5 into master Nov 8, 2022
@fcavazzoli fcavazzoli deleted the chore/use_cap_for_claimable_assets branch November 8, 2022 00:20
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.

feat: Update Claim Asset Flow
2 participants