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

Multi-network support in ERC20 driver #947

Merged
merged 3 commits into from
Jan 25, 2021

Conversation

Wiezzel
Copy link

@Wiezzel Wiezzel commented Jan 20, 2021

No description provided.

@Wiezzel Wiezzel self-assigned this Jan 20, 2021
@Wiezzel Wiezzel requested a review from a team January 20, 2021 17:03
@Wiezzel Wiezzel force-pushed the wiezzel/erc20-multi-network branch 6 times, most recently from b7aa372 to 71afa7d Compare January 21, 2021 16:12
@Wiezzel Wiezzel changed the title [WIP] Multi-network support in ERC20 driver Multi-network support in ERC20 driver Jan 21, 2021
@Wiezzel Wiezzel force-pushed the wiezzel/erc20-multi-network branch 3 times, most recently from 76428c2 to 0f86504 Compare January 21, 2021 21:58
@Wiezzel Wiezzel force-pushed the payment/multi-network branch from 9737ec6 to eb63d2f Compare January 22, 2021 10:10
@Wiezzel Wiezzel force-pushed the wiezzel/erc20-multi-network branch from 0f86504 to 1d8733e Compare January 22, 2021 10:36
Separated account funding from initialization. On rinkeby, fund command
obtains ETH and GLM from fucet. On mainnet, it displays a message to get
funds. Initialization in send mode requires non-zero balance of both
GLM and ETH.

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>
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.

LGTM!

Just some nitty comments, still approving!

core/payment-driver/gnt/src/gnt.rs Outdated Show resolved Hide resolved
&self,
address: &str,
) -> Pin<Box<dyn Future<Output = GNTDriverResult<String>> + 'a>> {
let address = utils::str_to_addr(address).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

.map_err()? instead of unwrap?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, that won't work. 😢 I could only do that within the async move block but I need the converted address before to create the futures.

Copy link
Author

Choose a reason for hiding this comment

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

I will move the type conversion one level up to remove unwrap.

address: &str,
network: Network,
) -> GNTDriverResult<BigDecimal> {
match network {
Copy link
Contributor

Choose a reason for hiding this comment

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

this match networkblock is repeated a lot, could be something lie self.get_network_driver(network).do_function().await

Copy link
Author

Choose a reason for hiding this comment

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

I'll give that a try.

_caller: String,
msg: Fund,
) -> Result<String, GenericError> {
log::info!("fund: {:?}", msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

msg should be logged on debug since its ugly, maybe a neat UX friendly message saying "Preparing to fund address {}..."

}

fn parse_platform(platform: String) -> Result<Network, GenericError> {
let parts: Vec<&str> = platform.split("-").collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

this stuff will not with with the metamask driver on your example, since {drivername}-{network}-{token} !== {platform}

Can leave it for now, maybe a note to our future selves would be nice

Copy link
Author

Choose a reason for hiding this comment

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

This code is not meant to be re-used by other drivers. It will be scrapped once erc20 driver is refactored to use base crate.

Applied review comments.

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>
@Wiezzel Wiezzel requested a review from maaktweluit January 25, 2021 10:28
@Wiezzel Wiezzel merged commit 1bc7e81 into payment/multi-network Jan 25, 2021
@Wiezzel Wiezzel deleted the wiezzel/erc20-multi-network branch January 25, 2021 10:43
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.

2 participants