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

GNT to GLM user facing refactor #1013

Merged

Conversation

pnowosie
Copy link
Contributor

@pnowosie pnowosie commented Feb 4, 2021

No description provided.

@pnowosie pnowosie requested review from Wiezzel, maaktweluit and a team February 4, 2021 12:55
Copy link

@Wiezzel Wiezzel left a comment

Choose a reason for hiding this comment

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

You missed some occurences:

core/payment-driver/zksync/src/driver.rs:106:

log::error!("NGNT transfer failed: {}", e);

Lots of "NGNT" in core/payment-driver/zksync/src/zksync/faucet.rs

accounts-example.json has deprecated 'ngnt' driver name.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
core/payment/examples/README.md Outdated Show resolved Hide resolved
@pnowosie
Copy link
Contributor Author

pnowosie commented Feb 5, 2021

You missed some occurences ...

I got fooled by new tool search UI - sorry about that :(

@pnowosie
Copy link
Contributor Author

pnowosie commented Feb 5, 2021

@Wiezzel question, do we do smth with counters

Copy link
Contributor

@maaktweluit maaktweluit left a comment

Choose a reason for hiding this comment

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

Great start!

Some "breaking" comments, please have a look

Also a grep GNT shows these files still have "wrong" occurrences:

  • docs/logging-guidelines.md = user facing, should be fixed this pr
  • core/payment/src/service.rs = platform is wrong, maybe for the second step of refactoring?

accounts-example.json Outdated Show resolved Hide resolved
@@ -8,7 +8,7 @@ cd core/payment
cp ../../.env-template .env
cargo run --example payment_api
```
To use GNT instead of dummy driver us `cargo run --example payment_api -- --driver=gnt` instead.
To use Erc-20 instead of ZkSync driver us `cargo run --example payment_api -- --driver=erc20` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you test this? i think it also needs platform
The invoice_flow also needs to get a matching platform as argument

Copy link
Contributor Author

@pnowosie pnowosie Feb 8, 2021

Choose a reason for hiding this comment

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

I improved the description above. And with Adam's help the code of payment_api example. Thank you for pointing this out.

payment_api with driver & platform parameter seems to start but it took long to get funded - I will let it run - will update whether it works if the experiment concludes. 💪

core/payment/examples/get_accounts.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Paweł Nowosielski and others added 4 commits February 5, 2021 13:27
Co-authored-by: Adam Wierzbicki <awierzbicki@golem.network>
Co-authored-by: Adam Wierzbicki <awierzbicki@golem.network>
@pnowosie
Copy link
Contributor Author

pnowosie commented Feb 5, 2021

* `core/payment/src/service.rs` = platform is wrong, maybe for the second step of refactoring?

Yes this is my concern as well: #1013 (comment)

@pnowosie pnowosie force-pushed the pnowosie/pay-203-gnt-to-glm-user-facing-refactor branch from 96f9c4b to 6c23776 Compare February 5, 2021 12:45
core/payment/examples/README.md Outdated Show resolved Hide resolved
core/payment/examples/README.md Outdated Show resolved Hide resolved
@Wiezzel
Copy link

Wiezzel commented Feb 8, 2021

Please update the counters initialization as well:

counter!("payment.amount.received", 0, "platform" => "NGNT");
// TODO: counter!("payment.amount.received", 0, "platform" => "ZKSYNC");
counter!("payment.amount.sent", 0, "platform" => "NGNT");
// TODO: counter!("payment.amount.sent", 0, "platform" => "ZKSYNC");

It should be:

 counter!("payment.amount.received", 0, "platform" => "erc20-rinkeby-tglm");
 counter!("payment.amount.received", 0, "platform" => "erc20-mainnet-glm");
 counter!("payment.amount.received", 0, "platform" => "zksync-rinkeby-tglm"); 
 counter!("payment.amount.received", 0, "platform" => "zksync-mainnet-glm"); 

 counter!("payment.amount.sent", 0, "platform" => "erc20-rinkeby-tglm");
 counter!("payment.amount.sent", 0, "platform" => "erc20-mainnet-glm");
 counter!("payment.amount.sent", 0, "platform" => "zksync-rinkeby-tglm"); 
 counter!("payment.amount.sent", 0, "platform" => "zksync-mainnet-glm"); 

pnowosie and others added 3 commits February 9, 2021 09:28
Co-authored-by: Adam Wierzbicki <awierzbicki@golem.network>
Co-authored-by: Adam Wierzbicki <awierzbicki@golem.network>
@pnowosie pnowosie requested a review from Wiezzel February 9, 2021 08:35
@pnowosie pnowosie merged commit 6f79958 into payments/beta-1 Feb 9, 2021
@pnowosie pnowosie deleted the pnowosie/pay-203-gnt-to-glm-user-facing-refactor branch February 9, 2021 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants