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

Update modelator to 0.2.0 #1249

Merged
merged 6 commits into from
Aug 9, 2021
Merged

Update modelator to 0.2.0 #1249

merged 6 commits into from
Aug 9, 2021

Conversation

jtremback
Copy link
Contributor

@jtremback jtremback commented Jul 29, 2021

Description

This updates to the new Modelator 0.2.0. Some caveats:

Let me know if this looks OK otherwise, and I can add the unclog file and other housekeeping stuff to this PR.


For contributor use:

  • Added a changelog entry, using unclog.
  • (not applicable) If applicable: Unit tests written, added test to CI.
  • (not applicable) Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

modules/tests/runner/mod.rs Show resolved Hide resolved
modules/tests/runner/mod.rs Outdated Show resolved Hide resolved
@jtremback jtremback changed the title Jehan/update modelator Update modelator to 0.2.0 Jul 29, 2021
@jtremback
Copy link
Contributor Author

Security audit is failing but that appears to be a Tendermint dep?

@adizere
Copy link
Member

adizere commented Aug 2, 2021

I'm not sure I understand the relation between this PR and the previously-opened one #812. Could you clarify if the current PR complete supersedes (so we can close and ignore) #812?

@adizere
Copy link
Member

adizere commented Aug 3, 2021

Security audit is failing but that appears to be a Tendermint dep?

Indeed, not exactly Tendermint dep, but fixing the security audit is blocked by prost & tonic (ref #1166 (comment))

@jtremback
Copy link
Contributor Author

jtremback commented Aug 3, 2021

I'm not sure I understand the relation between this PR and the previously-opened one #812. Could you clarify if the current PR complete supersedes (so we can close and ignore) #812?

We should probably wait for @andrey-kuprianov to weigh in, but #812 brings in a whole new API (the EventRunner API). It is also significantly behind master in other respects, which makes completing and merging it hard.

This PR uses the same API as the Modelator tests in master (the StepRunner API). This is a very straightforward PR, which just involved updating the modelator version and changing some function names etc. It unblocks my work on the ICS07 upgrade client mbt tests, since there were some bugs in the old modelator currently used on master.

We may want to finish #812 and switch over to the EventRunner API in the future, but IMO that is a separate task and this PR should be merged first.

@adizere
Copy link
Member

adizere commented Aug 4, 2021

I'm not sure I understand the relation between this PR and the previously-opened one #812. Could you clarify if the current PR complete supersedes (so we can close and ignore) #812?

We should probably wait for @andrey-kuprianov to weigh in, but #812 brings in a whole new API (the EventRunner API). It is also significantly behind master in other respects, which makes completing and merging it hard.

This PR uses the same API as the Modelator tests in master (the StepRunner API). This is a very straightforward PR, which just involved updating the modelator version and changing some function names etc. It unblocks my work on the ICS07 upgrade client mbt tests, since there were some bugs in the old modelator currently used on master.

We may want to finish #812 and switch over to the EventRunner API in the future, but IMO that is a separate task and this PR should be merged first.

Got it, thanks for the explanation Jehan, that's very helpful!

Copying @cezarad here if she wants to weight in by reviewing this PR.

@romac romac self-assigned this Aug 4, 2021
@romac romac changed the title Update modelator to 0.2.0 Update modelator to 0.2.0 Aug 4, 2021
@romac romac assigned romac and cezarad and unassigned romac Aug 4, 2021
@romac romac merged commit 7909c2d into master Aug 9, 2021
@romac romac deleted the jehan/update-modelator branch August 9, 2021 14:52
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* got rust compiling with new modelator, problems with the spec

* everything works, but I had to delete an assert I don't understand

* get rid of uneccesary clone, add unclog

* rustfmt

* remove outdated comment

* Update .changelog/unreleased/improvements/1249-update-modelator.md

Co-authored-by: Romain Ruetschi <romain@informal.systems>
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.

5 participants