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

Add recovery processes for the spec workflows. #9

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Sep 13, 2023

This change is Reviewable

@dzmitryhil dzmitryhil changed the title Add recovery for the workflows. Add recovery processes for the spec workflows. Sep 13, 2023
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)


spec/img/allocate-ticket.png line 0 at r1 (raw file):
image.png

I guess that you are aware but:
Since we use AccountSequence For recovery operations we will have to build txs a bit differently comparing to ticket txs


spec/img/allocate-ticket.png line 0 at r1 (raw file):
image.png

is recovery triggered by relayer or admin ? But not automatically by smart contract ?
Not clear in the diagram


spec/img/allocate-ticket.png line 0 at r1 (raw file):
image.png

Lets discuss after call.
I have just realized that we are not allowed to just allocated n ticket. There might be situation when we have a lot of pending txs and since total number of tickets couldn't be more than 250 we should allow to allocate up to 250 - (allocated_tickets + unused_tickets)

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)


spec/img/allocate-ticket.png line at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

image.png

I guess that you are aware but:
Since we use AccountSequence For recovery operations we will have to build txs a bit differently comparing to ticket txs

Right I know. The only difference actually is that we fill the different attribute in the transaction.


spec/img/allocate-ticket.png line at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

image.png

is recovery triggered by relayer or admin ? But not automatically by smart contract ?
Not clear in the diagram

By admin. The arrow goes from the admin.


spec/img/allocate-ticket.png line at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

image.png

Lets discuss after call.
I have just realized that we are not allowed to just allocated n ticket. There might be situation when we have a lot of pending txs and since total number of tickets couldn't be more than 250 we should allow to allocate up to 250 - (allocated_tickets + unused_tickets)

I didn't get the use case fully. Let's discuss after the call.

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)


spec/img/allocate-ticket.png line at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I didn't get the use case fully. Let's discuss after the call.

@ysv We need to discuss it still.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @ysv)


spec/spec.md line 53 at r1 (raw file):

All tokens issued on the coreum that can be bridged from the coreum to XRPL and back must have a representation on the
XRPL, managed by the multi-signing account. Such tokens should be registered by admin on the contract side with the
coreum denom and fees. The XRPL currency uniquely generated using the denom.

currency is uniquely...

@dzmitryhil dzmitryhil requested a review from a team as a code owner September 19, 2023 07:24
Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)


spec/spec.md line 53 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

currency is uniquely...

Done.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @keyleu, @miladz68, and @ysv)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @keyleu and @ysv)

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @keyleu)

@dzmitryhil dzmitryhil merged commit cd4a143 into master Sep 21, 2023
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.

4 participants