test: rewrite upgrade test as a sandbox integration tests #1186
Conversation
There was a problem hiding this comment.
Thank you.
I would like to see some more state added before the migration call (at least what we have in the pytest we are about to delete, but even better if we were to resolve #415 with this PR)
…ate_to_current-as-a-sandbox-integration-tests
There was a problem hiding this comment.
Thanks for all the work. I think we are getting close to merge, but there are still a few things we need to look at.
Hard blockers for me right now are the following:
- concerning large key event timeout #1186 (comment)
- use of
DomainId::legacy_ecdsa_id()in non-backwards compatibility tests #1186 (comment) - slight regression testing the upgrade functionality of the current contract #1186 (comment)
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn sign_and_validate( |
There was a problem hiding this comment.
Nice refactor, I like that we are breaking the logic out into smaller, re-usable functions!
Could we add a doc comment for this function?
There was a problem hiding this comment.
This function already existed before. Seems like the diff is getting messed up because of re-ordering. Feel free to create an issue to document.
There was a problem hiding this comment.
Feel free to create an issue to document.
I think the effort of opening an issue and following it with a PR is not justified for a doc comment that could be added as a broken-window fix.
But this was never a blocker for this PR, so, if you are happy without the comment, we don't need one.
| ) -> anyhow::Result<()> { | ||
| let status = submit_sign_request(request, contract).await?; | ||
|
|
||
| tokio::time::sleep(std::time::Duration::from_secs(3)).await; |
There was a problem hiding this comment.
Out of scope for this PR, but could we open an issue to get rid of this sleep?
There was a problem hiding this comment.
Sure, feel free to create cone.
|
Thank for the very in depth review. I believe all these hard blockers have been addressed now.
|
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn sign_and_validate( |
There was a problem hiding this comment.
Feel free to create an issue to document.
I think the effort of opening an issue and following it with a PR is not justified for a doc comment that could be added as a broken-window fix.
But this was never a blocker for this PR, so, if you are happy without the comment, we don't need one.
…ate_to_current-as-a-sandbox-integration-tests
DSharifi
deleted the
1185-rewrite-test_update_to_current-as-a-sandbox-integration-tests
branch
closes #1185