golemfactory / yagna-triage

Repository for issues which we don't yet know about enough to assign to proper repo
1 stars 0 forks source link

goerli shown on network list in golemsp status #171

Closed ederenn closed 1 year ago

ederenn commented 2 years ago

Name: ederenn

if it is a new network, and we are able to use it, should be mentioned in the papers (and maybe in the blogpost) if we are not planning to share info about it, it should be hidden on the list

    network               mumbai                        │
    │  amount (total)        1099.924860558341997779 tGLM  │
    │      (goerli)          0 tGLM                        │
    │      (mumbai)          0 tGLM                        │
    │      (rinkeby)         1000.006753314891127779 tGLM  │
    │      (zksync)          99.91810724345087 tGLM        │
    │                                                      │
    │  pending               0 tGLM (0)                    │
    │  issued                0 tGLM (0)
tworec commented 2 years ago

I'd recommend removing it, because when one will do

yagna payment init --sender --network goerli --driver zksync

yagna panics

[2021-12-01T17:01:03.917+0100 DEBUG ya_zksync_driver::driver] init: Init { address: "0x4c94f8883553ed92f7cffb01eab9968152291027", network: Some("goerli"), token: None, mode: SEND }
[2021-12-01T17:01:03.917+0100 DEBUG ya_zksync_driver::zksync::wallet] init_wallet. msg=Init { address: "0x4c94f8883553ed92f7cffb01eab9968152291027", network: Some("goerli"), token: None, mode: SEND }
[2021-12-01T17:01:03.917+0100 DEBUG ya_zksync_driver::zksync::wallet] get_wallet "0x4c94f8883553ed92f7cffb01eab9968152291027"
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "goerli"',
jiivan commented 2 years ago

I'd recommend removing it, because when one will do

yagna payment init --sender --network goerli --driver zksync

yagna panics

golemsp never asks yagna for such a thing:

https://github.com/golemfactory/yagna/blob/d7480f52369b3b4bee0ea4abc07e538b55251b8c/golem_cli/src/command/yagna.rs#L271-L272

Altough maybe yagna's payment shouldn't panic in this situation (a task for another issue).

My conclusion is that:

  1. golemsp won't cause such problem, because he has internal list of zksync compatible networks (command::yagna::ZKSYNC_DRIVER)
  2. yagna payment panics every time you try to initialize it with zkSync and incompatible network (polygon/mumbai/goerli)
  3. This issue is payment related (not provider)
  4. If we don't want to show goerli than IMHO it should be removed from core model: https://github.com/golemfactory/yagna/blob/d7480f52369b3b4bee0ea4abc07e538b55251b8c/core/model/src/payment.rs#L443-L444
tworec commented 2 years ago

yes, all the above is right.

I'd remove goerli entirely from the yagna codebase, including golemsp as per this issue.

I'd also fill an issue to fix panicking zksync payment driver, to be fixed later on (eg. in patch release)

tworec commented 2 years ago

I think this is also for the patch release. No need to be fixed immediately.

jiivan commented 2 years ago

I'd also fill an issue to fix panicking zksync payment driver, to be fixed later on (eg. in patch release)

I've created an issue: https://github.com/golemfactory/yagna/issues/1740

maaktweluit commented 2 years ago

This will remove the option to test enter / exit from mumbai, asked 2rec about this

maaktweluit commented 2 years ago

@tworec what was the final descision on enter / exit? will we keep goerli?

tworec commented 2 years ago

Together with @prekucki we've decided to support and keep goerli. In order to do that we need:

jiivan commented 2 years ago

Core team has created two issues related to this:

Leaving the handbook enrichment & verifying ya*api compatibility to @golemfactory/ya-sdk

golmek commented 1 year ago

Closing ticket as Goerli is part of default network since YAGNA 0.12.2