-
Notifications
You must be signed in to change notification settings - Fork 75
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
pre-release: ASI upgrade #392
Conversation
Visit the preview URL for this PR (updated for commit 2b8f849): https://fetch-docs-preview--pr392-feat-asi-upgrade-con-pyqiamkm.web.app (expires Thu, 20 Jun 2024 10:08:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f2de39fd4e81249941960b74fbab0a62d90d69f8 |
@@ -177,12 +177,12 @@ class TestContractRestAPI(TestContract): | |||
|
|||
def _get_network_config(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some integration tests we deploy sample contracts on testnet in every run! This leads to adding unnecessary data there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add better support for simulating txs for tests like these, because I think in almost all cases, that is all that really needs to be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -125,8 +140,8 @@ def fetchai_mainnet(cls) -> "NetworkConfig": | |||
chain_id="fetchhub-4", | |||
url="grpc+https://grpc-fetchhub.fetch.ai", | |||
fee_minimum_gas_price=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We have been considering setting this default minimum gas price here to non-zero value (the same value as for testnet). Even though this is not enforceable, practically speaking, this is what we want - if nothing else, then at least defaulting to non-zero Tx gas price value will make it less straightforward for users to use zero Tx fee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now (for ASI umgrade release), please ignore my comment above. We will touch on thsese kind of things in later releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: In this sense (= not now), we shall also extend the NetworkConfig
class with address prefix data member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create the feature branch feat/asi_upgrade
on the tip of the current main
branch and set this feat/asi_upgrade
branch as landing branch opf this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed changes
asi
fetch
addresses toasi
addresses in examples, tests, and docs.Do we know new chain-id for
fetchhub-4
?Issues
Links to any issues resolved.
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply.main
branch.If applicable
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did, what alternatives you considered, etc.