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

Cleanup resourceID/chainID/executeProposal tests #11

Merged
merged 9 commits into from
Jul 29, 2021

Conversation

kmadk
Copy link
Contributor

@kmadk kmadk commented Jul 28, 2021

No description provided.

@kmadk kmadk requested a review from drewstone July 28, 2021 21:03
Copy link
Contributor

@drewstone drewstone left a comment

Choose a reason for hiding this comment

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

Would recommend copying @nepoche and i's naming convention on branches. If you're confused on process, don't be afraid to ask questions in slack and/or to us directly. Naming conventions should be copy-able by looking at our work/PRs.

As far as renaming goes in these tests, it all seems fine to shift origin to source. However, I think semantically the tests aren't correct. It seems we:

  1. Create a linkable anchor on the source chain
  2. Vote on update proposals created with source chain ID on source chain anchor

Doesn't this seem wrong? We should be creating update proposals for dest chain ID no? The linkable anchor on source chain should be bridged (addEdge/updateEdge) with the dest chain. Can you verify that these tests include that?

@kmadk
Copy link
Contributor Author

kmadk commented Jul 29, 2021

As far as renaming goes in these tests, it all seems fine to shift origin to source. However, I think semantically the tests aren't correct. It seems we:

  1. Create a linkable anchor on the source chain
  2. Vote on update proposals created with source chain ID on source chain anchor

Doesn't this seem wrong? We should be creating update proposals for dest chain ID no? The linkable anchor on source chain should be bridged (addEdge/updateEdge) with the dest chain. Can you verify that these tests include that?

All of the proposal tests have the Bridge instance instantiated on destChainID along with a corresponding handler and anchor on destChainID. The vote and execute proposal calls are all voting and acting through the bridge on the destChainID chain after a deposit happens on the source chain so the update proposals are on the correct chain for all these tests. This design carries over from the chainBridge depositProposal tests that I didn't change.

Thank you for the help with the naming convention I have been a bit messy with that I understand. For future reference should a branch like this be called "kelton-clean-up" or what do you think would be a proper name.

@drewstone drewstone merged commit ace88ef into main Jul 29, 2021
@drewstone drewstone deleted the clean-resourceID/chainID/executeProposalTests branch July 29, 2021 14:47
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.

2 participants