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: Remove data results from post result sudo msg #23

Merged
merged 10 commits into from
Oct 10, 2024
Merged

Conversation

hacheigriega
Copy link
Member

Motivation

Removing data results fields from the PostResult sudo message since the contract no longer needs them. This chain repo implements a data results store on the chain side instead: sedaprotocol/seda-chain#357

@gluax
Copy link
Contributor

gluax commented Sep 24, 2024

Do we even need this message anymore? Or is it just so the chain notifies the contract to remove them from the data request pool? Do we still need a query to get a data result from the contract?

@hacheigriega
Copy link
Member Author

Do we even need this message anymore? Or is it just so the chain notifies the contract to remove them from the data request pool? Do we still need a query to get a data result from the contract?

Right it would be just so that the contract can remove the items from the data request pool.

And good point that data results would have to be queried on the chain side instead. I removed the query here.

@gluax
Copy link
Contributor

gluax commented Sep 24, 2024

Got it. Let me help with fixing the failing CI stuff, but I would like to wait to merge this till we make the changes in other affected places and can test it with the test suite or something. Or maybe I'm being too paranoid, but let's see what others think?

@hacheigriega
Copy link
Member Author

Got it. Let me help with fixing the failing CI stuff, but I would like to wait to merge this till we make the changes in other affected places and can test it with the test suite or something. Or maybe I'm being too paranoid, but let's see what others think?

Yes for sure and thanks. I thought I could handle Rust..

@gluax
Copy link
Contributor

gluax commented Sep 24, 2024

No one handles rust, rust handles you 😆

@hacheigriega
Copy link
Member Author

hacheigriega commented Sep 24, 2024

No one handles rust, rust handles you 😆

I also got stuck while making corresponding changes in the contract. It's probably much quicker that you just take care of it.. I think the module should just take care of hashing data results and emitting events about them. Initially I was hoping to get the data result hash as a return value of the sudo call, but I don't think this is possible.

@gluax
Copy link
Contributor

gluax commented Sep 24, 2024

No one handles rust, rust handles you 😆

I also got stuck while making corresponding changes in the contract. It's probably much quicker that you just take care of it.. I think the module should just take care of hashing data results and emitting events about them. Initially I was hoping to get the data result hash as a return value of the sudo call, but I don't think this is possible.

Hm, yeah, we'd need to match the hashing that was done here on the go side, but that should be fine. The contract could do the hashing, but it would require not changing the Sudo method as it would still need the result to do the hash.

I can make the corresponding changes on the other sides, I'll focus on that tomorrow if that's ok!

@hacheigriega
Copy link
Member Author

Hm, yeah, we'd need to match the hashing that was done here on the go side, but that should be fine. The contract could do the hashing, but it would require not changing the Sudo method as it would still need the result to do the hash.

I can make the corresponding changes on the other sides, I'll focus on that tomorrow if that's ok!

Sounds good! And I am working on the chain side.

@gluax gluax force-pushed the hy/sudo-refactor branch from 036b0b9 to 160c7ed Compare October 3, 2024 15:34
@gluax gluax changed the base branch from main to refactor/types-and-renames October 3, 2024 15:46
@gluax gluax requested a review from Thomasvdam October 3, 2024 16: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.

Lgtm

Base automatically changed from refactor/types-and-renames to main October 10, 2024 14:20
@gluax gluax merged commit dc25bbc into main Oct 10, 2024
2 checks passed
@gluax gluax deleted the hy/sudo-refactor branch October 10, 2024 14:21
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