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

Refactor: results not stored on contract #217

Merged

Conversation

gluax
Copy link
Collaborator

@gluax gluax commented Sep 25, 2024

Motivation

To simplify and store results on the chain and not on the contract.

Explanation of Changes

  • Updates common.
  • Removes the query for a result.
  • Removes storage of data results.
  • Renames PostResult stuff to RemoveRequest.

Testing

Tests no longer check result is stored and all pass... but maybe I should change them to check DR_ID no longer exists in the contract though? Thoughts/opinions?

Related PRs and Issues

Waiting on sedaprotocol/seda-common-rs#23 to be merged.

Blocked until #219 is merged.

I'd also like to test these changes with the test suite once we have the changes on the chain as well :).

@gluax gluax self-assigned this Sep 25, 2024
@gluax gluax requested review from Thomasvdam, mariocao, FranklinWaller and hacheigriega and removed request for Thomasvdam September 25, 2024 15:13
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

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

Possibly stupid idea, but I worry that the scope of this change is a little too big as we'll have to coordinate across basically all components (Rust solver, chain, and explorer).

Maybe we can make the transition a little easier by supporting both flows for a while, that will allow us to merge changes and not have to worry about making sure everything is deployed at the same time. Although with the hackathon going on we're limited with deployments anyway so 🤷

Anyway, my proposal would be something like the following:

  • We keep the post_result result method as is.
  • We add a sudo method for removing a DR from the pool. I think have post_result only removing a DR doesn't make a lot of sense naming wise. This shouldn't fail if the DR doesn't exist (since post_result may have already removed it).
  • We keep the result queries so the Rust solver can keep doing its thing.
  • We do remove the check whether a new DR already has a result.

This should allow the chain, solver, and explorer to migrate to the new flow at their own pace and once everything is ready we can remove the post_result method entirely.
Of course it's a little more work in total, but I think it's worth it to ensure everything keeps working and we're not all working in silos again until all changes are implemented across all components only to then realise it doesn't work nicely together.

@gluax
Copy link
Collaborator Author

gluax commented Sep 26, 2024

Possibly stupid idea, but I worry that the scope of this change is a little too big as we'll have to coordinate across basically all components (Rust solver, chain, and explorer).

Maybe we can make the transition a little easier by supporting both flows for a while, that will allow us to merge changes and not have to worry about making sure everything is deployed at the same time. Although with the hackathon going on we're limited with deployments anyway so 🤷

Anyway, my proposal would be something like the following:

* We keep the `post_result` result method as is.

* We add a sudo method for removing a DR from the pool. I think have `post_result` only removing a DR doesn't make a lot of sense naming wise. This shouldn't fail if the DR doesn't exist (since `post_result` may have already removed it).

* We keep the result queries so the Rust solver can keep doing its thing.

* We do remove the check whether a new DR already has a result.

This should allow the chain, solver, and explorer to migrate to the new flow at their own pace and once everything is ready we can remove the post_result method entirely. Of course it's a little more work in total, but I think it's worth it to ensure everything keeps working and we're not all working in silos again until all changes are implemented across all components only to then realise it doesn't work nicely together.

My concern is a bit of the opposite:

If we leave both in instead of ripping off the bandaid, I'm scared it may cause us to miss things with the new approach. I.e. there is a bug with the new stuff, and we don't know because the old things still exist and are used.

We can also test a successful dr with the test suite, which makes me feel more ok going this route.

My perspective is we could always roll this back easily(cherry-picking) if need be.

Pros and cons to both ways, ig it's just a matter of which cons we'd prefer.

I do 100% agree we should rename post_result tho

@gluax
Copy link
Collaborator Author

gluax commented Sep 27, 2024

I will return to this since it will sit for a bit and do the other changes first. This way I can rebase them all at once with this one :).

@gluax gluax changed the base branch from main to refactor/types-and-renames October 3, 2024 15:46
@gluax gluax force-pushed the refactor/results-not-stored-on-contract branch from 91b748d to eb57376 Compare October 3, 2024 16:02
@gluax gluax requested a review from Thomasvdam October 3, 2024 16:13
Copy link
Member

@hacheigriega hacheigriega left a comment

Choose a reason for hiding this comment

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

LGTM

@gluax gluax merged commit ede475b into refactor/types-and-renames Oct 10, 2024
2 checks passed
@gluax gluax deleted the refactor/results-not-stored-on-contract branch October 10, 2024 14:28
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