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

Implement expiration data for application links #516

Closed
RiccardoM opened this issue Jun 25, 2021 · 6 comments · Fixed by #723
Closed

Implement expiration data for application links #516

RiccardoM opened this issue Jun 25, 2021 · 6 comments · Fixed by #723
Assignees
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist

Comments

@RiccardoM
Copy link
Contributor

Feature description

Currently when a user links their centralized application with the Desmos profile, the created link contains a timestamp of when it has been created. Since centralized applications username can be switched and sold, we should implement a sort of "expiration date" on links. This means that after a certain amount of time passes, the link will be automatically marked as invalid, and the user has to connect it again in order to keep it valid.

Implementation proposal

The implementation should be very simple. What we can do is:

  1. Add a parameter inside the x/profiles parameters set, identifying how long a link should be considered valid from when it has been created.
  2. Inside the EndBlocker, check for links that are expired and update their state to set them as such.

Please @bragaz @dadamu tell me if you see any problem with this implementation or better alternatives.

@RiccardoM RiccardoM added the kind/new-feature Propose the addition of a new feature that does not yet exist label Jun 25, 2021
@RiccardoM RiccardoM self-assigned this Jun 25, 2021
@leobragaz
Copy link
Contributor

The implementation looks good for me but I was actually wondering if it can be done by:

  1. Checking the AcceptDTagTransferRequest Tx made
  2. Invalidate the links of the two users that transferred the DTags

I'm thinking this because maybe it become tedious to let the re-link the apps when they expires.
But actually this expiration could (should?) act as a security measure and then my implementation loses its meaning

@RiccardoM
Copy link
Contributor Author

@bragaz The problems does not sussist with DTag transfers. An application is linked to a profile using the address, not the DTag. This means that DTag can change whenever they want and application links will still be valid for the linked profiles since they are verified by signing something with the profile private key.

The idea of expiring application links is to solve the current situation:

  1. Alice links her Twitter username @AliceInWonderland
  2. Alice sells her Twitter username to the creator of "Alice in Wonderland"

In this case, the link should expire at some time so that she is required to verify again and does not keep having associated to her profile a Twitter account that she used to own and she does not own anymore.

@leobragaz
Copy link
Contributor

Oh then I misunderstood at the start, I was thinking about the DTag, now it makes sense.
Then I think your proposal is more than good for handle this!

@dadamu
Copy link
Contributor

dadamu commented Jun 28, 2021

Just I have a question. After centralized applications username is switched and sold, the user can send a tx to unlink the app-link, Is it not enough?

By your proposal, Desmos should check all app-links in the EndBlocker phase, it would be a cost thing. I think it is better just adding a field expired_time, then checking whether app-link is expired in every operation related to it.

@RiccardoM
Copy link
Contributor Author

Just I have a question. After centralized applications username is switched and sold, the user can send a tx to unlink the app-link, Is it not enough?

This isn't enough cause we can't rely on users to perform these kind of actions. They might simply forget to do it or stop using Desmos before changing their application username. This would cause Desmos to expose a way users can use to impersonate other people.

By your proposal, Desmos should check all app-links in the EndBlocker phase, it would be a cost thing. I think it is better just adding a field expired_time, then checking whether app-link is expired in every operation related to it.

You are right, it might become too expensive. Thanks to your idea of adding a field, I have thought about another implementation. Instead of using a time (which is hard to deal with inside our chain), what we can do is set a validity time in blocks. This way when a link gets created we can store a new entry that is something like

ExpirationPrefix + BlockNumber + ClientID -> ClientID

This way, inside the EndBlocker we can simply:

  1. Iterate over all the ExpirationPrefix + CurrentBlockHeight
  2. Set all the links associated with the different ClientID as expired

This should be very efficient by iterating and editing only the links that are supposed to be changed. what do you think?

@RiccardoM RiccardoM added this to the v0.18.0 milestone Jun 30, 2021
@dadamu dadamu self-assigned this Jul 14, 2021
@dadamu
Copy link
Contributor

dadamu commented Jul 15, 2021

There is an issue that the ExpirationKey cannot be deleted that when deleting the application link since there is no clue to get the expiry block height from a link. So, If the user re-create the link after deleting, the expiry block height cannot be updated to the new state.

There is a way to solve the problem, building a key manager for application link, like:

struct ApplicationLinkKeyManager type{
  UserApplicationLinkKey       []byte
  ApplicationLinkClientIDKey []byte
  ApplicationExpirationKey    []byte
}

Within this solution, all the key can be deleted and created easily by manager, so does the application expiration key.

The other way is to add expiration block height into ApplicationLink, it is much easier than the above proposal. I would go with it first. (7/16 Upadated)

What do you think? @RiccardoM @bragaz

@RiccardoM RiccardoM removed this from the v2.0.0 milestone Sep 30, 2021
mergify bot pushed a commit that referenced this issue Jan 12, 2022
## Description

This PR introduce the draft of the 4th ADR related to Expiration time of application links.
Discussed before here: #516.

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [ ] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist
Projects
None yet
3 participants