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

[WIP] pallet-anchors: add precommit deposit #879

Merged
merged 20 commits into from
Aug 31, 2022
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jul 21, 2022

New changes (Specs):

  • Add a Pre-commit deposit.
  • Auto pre-commit eviction at commit() if successful.
  • Refactor evict_pre_commit()
  • Added benchmarks.

Blocked by #890

Tasks:

Closes #865
Closes #706

Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Code looks good!

runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm self-assigned this Jul 22, 2022
Copy link
Contributor

@mikiquantum mikiquantum left a comment

Choose a reason for hiding this comment

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

I am still on the fence about the deposit vs fee here bc the archiving node nature.
A few comments:

  • I would add the unreserve as well in the commit call, when there is an existing precommit. The reason being that for most of the flows that call precommit before will call commit a few minutes later max (most likely seconds). This comes with extra cost, of triggering manually a eviction of the precommits, which is not straight forward since the precommits to be evicted are indexed by buckets by number and idx. But is a really bad user experience if when following the happy path a user will have to wait until someone calls (or themselves) the evict bucket call.
  • The issue of not storing the PreCommitDeposit in the precommit itself is that if we unreserve after that amount has been changed by a runtime upgrade, then the user would get less/more (depending on the lock) of what it was reserved. If we go with this approach we need to store the precommit balance in the storage.

Thinking further on this, this is becoming a bit more complex, due to the nature of the eviction buckets for precommits. I am leaning more towards implementing the flat extra cost (non returnable fee) for calling the PreCommit function instead. If later on, we start seeing more usage of this function, then we can reconsider. I would keep it simple for now.

pallets/anchors/src/lib.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm force-pushed the precommit-deposit branch 2 times, most recently from d5efb50 to 4538d96 Compare August 9, 2022 06:56
/// These funds will be unreserved once the user make the [`commit()`] succesfully
/// or call [`Pallet::evict_pre_commits()`]
#[pallet::constant]
type PreCommitDeposit: Get<BalanceOf<Self>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably this type can be removed once pallet_fees be updated with a storage key-enum based.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikiquantum Do we want to use a key from the pallet key for the pre-commit deposit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would say so, since we already have the setup for it.

@lemunozm lemunozm dismissed a stale review via ade98d5 August 25, 2022 06:08
@lemunozm lemunozm force-pushed the precommit-deposit branch 3 times, most recently from eecc130 to f900f06 Compare August 29, 2022 08:06
assert_eq!(<PreCommits<T>>::iter_values().count(), 0);
}

evict_anchors {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is really difficult to accuracy benchmark this. It has two big parameters that affect the computation of this call. First, the MAX_LOOP_IN_TX, and second the number of days from the last evict_anchors call. The first one is handled. But the second one has no limit, and I do not know how to deal with it. Anyway, I don't think it is an issue, since this call is not idempotent. I mean, only the first call could have a big amount of computation, the next ones not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the evict_anchor logic and simplify it to be called externally per anchor_id like we do with the evict_precommit. Lets chat more about it offline.

@lemunozm lemunozm merged commit 2613b7d into parachain Aug 31, 2022
@mikiquantum mikiquantum deleted the precommit-deposit branch August 31, 2022 10:05
@@ -504,6 +504,7 @@ pub enum Adjustment<Amount> {
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
pub enum FeeKey {
CommitAnchor,
PreCommitDeposit,
Copy link
Contributor

Choose a reason for hiding this comment

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

I should have looked at this earlier, my bad, but wouldn't it make more sense to call these keys something like

AnchorCommitDeposit
AnchorPreCommitDeposit

Since there could possibly be other pre-commits in the future not related to anchors? And for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Even we can add a sub-enum per pallet I think. Something like:

FeeKey::Anchor(Anchor::CommitDeposit);

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.

Add pre_commit deposit Benchmarks pallet-anchors
4 participants