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

SlashAcks should not be persisted as string #728

Closed
shaspitz opened this issue Feb 9, 2023 · 6 comments
Closed

SlashAcks should not be persisted as string #728

shaspitz opened this issue Feb 9, 2023 · 6 comments
Assignees
Labels
good first issue Good for newcomers type: refactoring Code refactoring type: tech-debt Slows down development in the long run

Comments

@shaspitz
Copy link
Contributor

shaspitz commented Feb 9, 2023

Problem

A "slash ack" is a string which is relayed between provider and consumer to reset the consumer's outstanding downtime flag. SlashAcks are always consumer consensus addresses, but we persist and send these slash acks as a string type. This design is error prone because:

  1. It does not leverage type safety (see SlashAck sent to consumer is wrong #693)
  2. There are multiple formats to convert a consensus address to string

Closing criteria

Make slash acks persisted and ser/deser as pb types, not strings

Problem details

A fix to this issue should be built on top of #725. Since slash acks can be persisted as consumer cons address pb types

@shaspitz shaspitz added type: tech-debt Slows down development in the long run type: refactoring Code refactoring labels Feb 9, 2023
@shaspitz
Copy link
Contributor Author

shaspitz commented Feb 10, 2023

Note: this will likely require a state migration so I'm going push it off for a future ICS version

@mpoke mpoke added this to the ICS v1.0.1 milestone Feb 23, 2023
@shaspitz shaspitz added the good first issue Good for newcomers label Mar 2, 2023
@shaspitz
Copy link
Contributor Author

shaspitz commented Mar 2, 2023

@yaruwangway, now that #725 is merged, this issue is able to be solved if you're interested! Lmk if you want more options for a first issue

@mpoke mpoke removed this from the ICS v1.0.1 milestone Mar 5, 2023
@ameya-deshmukh
Copy link

Hi @smarshall-spitzbart is this still available?

@shaspitz
Copy link
Contributor Author

shaspitz commented May 4, 2023

Hi @smarshall-spitzbart is this still available?

@ameya-deshmukh sorry for the late response! Yes this issue is still open and noone is currently working on it. Are you interested in contributing?

@MSalopek
Copy link
Contributor

MSalopek commented Jul 4, 2023

The issue has become state breaking in the meantime.
Any changes in those parts will require a state migration, I'm not sure how feasible this is.

@shaspitz
Copy link
Contributor Author

shaspitz commented Jul 4, 2023

A solution doesn't have to be state breaking, but it would naturally have to break the wire. Solution could be implemented in such a way that it is backwards compatible tho.

Now that ICS is in production I agree that this issue is most likely not feasible. Unfortunately it is a source of technical debt.

@yaruwangway it seems we could close this issue by simply making the API for AppendSlashAck, GetSlashAcks, SetSlashAcks type safe, and remove the TODOs. The wire can keep string types to avoid complications

@yaruwangway yaruwangway self-assigned this Jul 20, 2023
@shaspitz shaspitz closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: refactoring Code refactoring type: tech-debt Slows down development in the long run
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

5 participants