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

Consistent variable names #230

Merged
merged 14 commits into from
Jan 12, 2024
Merged

Consistent variable names #230

merged 14 commits into from
Jan 12, 2024

Conversation

cam-schultz
Copy link
Contributor

Why this should be merged

Fixes #189

How this works

Renames vars

How this was tested

CI

How is this documented

N/A

@cam-schultz
Copy link
Contributor Author

The vulnerability reported by Snyk is coming from the avalanche-cli installation in the testing container. It's fixed in the repo, but we'll have to wait for the next avalanche-cli release for this to go away. Since this container is used for testing only, this is not a blocker.

geoff-vball
geoff-vball previously approved these changes Jan 12, 2024
@michaelkaplan13
Copy link
Collaborator

michaelkaplan13 commented Jan 12, 2024

There are a few more places that we should update:
image
image

@michaelkaplan13
Copy link
Collaborator

We should also rename DEFAULT_ORIGIN_BLOCKCHAIN_ID to DEFAULT_SOURCE_BLOCKCHAIN_ID.

@cam-schultz
Copy link
Contributor Author

There are a few more places that we should update:

We should also rename DEFAULT_ORIGIN_BLOCKCHAIN_ID to DEFAULT_SOURCE_BLOCKCHAIN_ID.

Made these changes.

@michaelkaplan13
Copy link
Collaborator

Looks like originChainID is used in a few places still.

@cam-schultz
Copy link
Contributor Author

Looks like originChainID is used in a few places still.

Whoops, fixed now. I also updated destinationChainID -> destinationBlockchainID

@michaelkaplan13
Copy link
Collaborator

Final few nits. In the three cases below, can we update to BlockchainID (either adding the "Block" or lower-casing the C in "Chain")

image

@cam-schultz
Copy link
Contributor Author

Final few nits. In the three cases below, can we update to BlockchainID (either adding the "Block" or lower-casing the C in "Chain")

Sure thing. Also renamed getBlockChainId.py -> getBlockchainId.py

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

🚀

Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

LGTM

@cam-schultz cam-schultz merged commit bedf79b into main Jan 12, 2024
14 of 15 checks passed
@cam-schultz cam-schultz deleted the consistent-var-names branch January 12, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consolidate origin vs source blockchainID and senderAddress
4 participants