feat: add beta-4 target, and point --testnet to beta-4 network for forc-deploy #4974
Conversation
kayagokalp
added
enhancement
|
We should point this PR to master once we have #4972 merged. |
|
In the future is there a way we could dynamically update this via a config file we host somewhere (ie. github) instead of needing to make new releases of forc? |
Yeah that is definitely a better idea. Opened #4975. |
kayagokalp
force-pushed
the
kayagokalp/4973
branch
from
9494cf2 to
b354148
Compare
Co-authored-by: Joshua Batty <[email protected]>
| testnet: false, | ||
| gas: Gas { | ||
| price: 1, | ||
| limit: 100000000, |
There was a problem hiding this comment.
Added some unit tests to show the current behavior - still don't understand the logic behind overriding the gas limit for beta-4.
There was a problem hiding this comment.
Thanks so much for adding the tests 💯 I am not sure why this is only needed for beta4 but the default max gas limit returns:
Error: Decode error: Custom { kind: Other, error: "Response errors; TransactionGasLimit" }
This is not the case for other beta networks.
If i change the default value, beta-3 and beta-2 would break, so I patched it only for beta-4 and if users provide gas_limit their gas_limit will override our preset value (unless they specified the max value, but again providing max value for gas limit returns the error above). This is not ideal but it enables to deploy without specifying any other flags.
Also I imagine this feature is only for beta networks for mainnet we would require users to specify --gas-limit and --gas-price by themselves (or if there is a way to estimate gas-limit using SDK, we can explore that later on as well)
There was a problem hiding this comment.
I'm just wondering if there's a more appropriate gas limit than 1. Does a gas limit of 1 work actually work for any transactions?
We might want to fix the issue with the default gas limit in forc-tx rather than overriding it in forc-deploy.
There was a problem hiding this comment.
We now ask the test net for min gas price if the user left it as 0 by default. The testnet returns the accepted min gas price and we use that instead of 0 so that the transaction is valid.
We do the same thing for gas_limit if they left as the default max value, we ask the chain the valid max value and use it instead, so that we do not exceed the limit set by the node.
I think that while deploying to test net we can override this as we want the transactions to be valid by default and if the user choses to be providing the extremes we can use the extreme set by the node instead. Maybe I can add a warning to notify them?
There was a problem hiding this comment.
On a second thought modiying this rn to make sure we do not override any user given data. If the user wants to override the default data returned by the node they can. Maybe they would want to test some stuff around that
| create | ||
| .gas | ||
| .limit | ||
| .unwrap_or(fuel_tx::ConsensusParameters::DEFAULT.max_gas_per_tx), |
There was a problem hiding this comment.
Should we be using the defaults from the node here as well?
There was a problem hiding this comment.
Actually it seems like forc-tx doesn't specify the node, so there's no way to use the node's defaults
Pull request was closed
…4991) ## Description Closes #4974 Added the beta-4 target and updated documentation (from #4974) Refactored part of forc-deploy and added unit tests. - No longer mutating the Command, so the Command is always whatever the user gave us - Using the default values from the node itself via API call, rather than hardcoded constants - The values of `Gas` are now optional, so we only override them when the user hasn't specified anything Other changes: - deployment works using the urls without `/graphql` prefixes, so I updated the constants to remove the prefix. This is shown to the user and it looks cleaner without it. - Added coloring and consistent styling for errors and warnings to printed to the console. It looks like this now (previously no color) - `deploy`, `run`, and `submit` all now have the same options for specifying the node (node_url, testnet, and target), capture in the `TargetNode` struct. They use the same helper functions to extract the node url and determine gas limit & price.  ## Checklist - [ ] I have linked to any relevant issues. - [ ] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: kayagokalp <[email protected]> Co-authored-by: Joshua Batty <[email protected]>
Description
closes #4973.
waiting for #4972 to be merged first.
With this PR, we are now adding
--target beta-4and alias--testnetto--target beta-4so that users can deploy to beta-4 with just providing--tesnetflag.