per-container payment config #556
Conversation
johny-b
changed the title
goth/configuration.py
Outdated
| @@ -292,6 +298,10 @@ def load_yaml( | |||
| name = node["name"] | |||
| type_name = node["type"] | |||
| use_proxy = node.get("use-proxy", False) | |||
|
|
|||
| payment_config_name = node.get("payments", DEFAULT_PAYMENT_CONFIG_NAME) | |||
There was a problem hiding this comment.
I find the name payments ambiguous in this context. How about payment_platform or payment_config?
| def payment_fund( | ||
| self: CommandRunner, payment_driver: PaymentDriver = DEFAULT_PAYMENT_DRIVER | ||
| ) -> None: | ||
| def payment_fund(self: CommandRunner, payment_driver: str) -> None: |
There was a problem hiding this comment.
Perhaps instead of taking a payment_driver string argument this should accept a PaymentConfig object? I think this would be slightly easier to use from the API's end user's perspective.
| """Run `<cmd> payment fund` with optional extra args.""" | ||
| args = make_args("payment", "fund", driver=payment_driver.name) | ||
| args = make_args("payment", "fund", driver=payment_driver) | ||
| self.run_command(*args) | ||
|
|
||
| def payment_init( |
There was a problem hiding this comment.
Same as above, maybe let's use a PaymentConfig object here? In this case we could use it instead of payment_driver and network parameters.
| @@ -110,15 +104,15 @@ def payment_init( | |||
|
|
|||
| def payment_status( | |||
There was a problem hiding this comment.
Same as above, if we decide to use PaymentConfig in these calls, let's change this one as well for consistency.
| @@ -50,6 +51,7 @@ def __init__( | |||
| self, | |||
| name: str, | |||
| probe_type: Type["Probe"], | |||
| payment_config: PaymentConfig, | |||
There was a problem hiding this comment.
Since this is a public field, please add its type definition to the class (like for other fields in this class).
|
@zakaprov I left
IOW, I'd rather not entangle those two parts of But I'm not super-sure about this, so if you insist, I'll change that : ) |
goth/configuration.py
Outdated
| @@ -292,6 +299,12 @@ def load_yaml( | |||
| name = node["name"] | |||
| type_name = node["type"] | |||
| use_proxy = node.get("use-proxy", False) | |||
|
|
|||
| payment_config_name = node.get( | |||
| "payment_config", DEFAULT_PAYMENT_CONFIG_NAME | |||
There was a problem hiding this comment.
Apologies for the confusion, but I think for the sake of consistency with other goth-config.yml fields the name of the field itself should be payment-config (hyphen instead of underscore).
There was a problem hiding this comment.
Good find. I should have thought about it.
cbb2254
Resolves #554
VISIBLE CHANGES:
goth-config.ymlhas optional key "payments" with a payment config name. Allowed names are now "erc20", "zksync" and "polygon". If "payments" is not specified, default value "zksync" will be used.zksyncanderc20were initialized). This shouldn't be hard to change if we want it one day.INTERNAL CHANGES:
goth/payment_config.py. This required some additional args here and there.PaymentDriverenum - I think it's not really useful after the change in previous point. I'm not 100% sure about this, but if we want it back than why not alsoPaymentNetworkenum?NOTE: The only thing I checked (outside of the
gothrepo) is that:yapapi) works with this versionpayments: erc20ingoth-config.ymlinyapapitests we need--payment-driver erc20 --payment-network mainnetintest_run_blenderpayments: erc20, blender uses only this providerAnd as far as I understand, this is what we wanted.
Recommended reading start:
goth/payment_config.py.Also, commits are messy, there's no use in reading them one by one.