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

Update serde + dynamically fetch XRP credentials #254

Merged
merged 4 commits into from Aug 29, 2019

Conversation

@emschwartz
Copy link
Collaborator

commented Aug 29, 2019

I needed to update serde to get #247 working locally.

@emschwartz emschwartz requested review from bstrie and dora-gt Aug 29, 2019

@emschwartz emschwartz force-pushed the es-update-serde branch from 9653595 to 31e34c8 Aug 29, 2019

test(xrp): get new testnet credentials each time
the test was failing because the testnet was reset

@emschwartz emschwartz force-pushed the es-update-serde branch from 31e34c8 to 7c16585 Aug 29, 2019

@emschwartz emschwartz requested a review from gakonst Aug 29, 2019

"rGCUgMH4omQV1PUuYFoMAnA7esWFhE7ZEV",
"sahVoeg97nuitefnzL9GHjp2Z6kpj",
&node2_xrp_credentials.address,
&node2_xrp_credentials.secret,

This comment has been minimized.

Copy link
@bstrie

bstrie Aug 29, 2019

Collaborator

Is this intended to be a part of this commit?

.json()
.expect("Got unexpected response from XRP testnet faucet");
body.account
}

This comment has been minimized.

Copy link
@bstrie

bstrie Aug 29, 2019

Collaborator

The PR only mentions updating Serde, but it looks like there's some additional changes in this branch. Intentional?

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 29, 2019

Member

Yeah seems like after all these E2E tests the faucet accounts I had hard-coded stopped working. Modified the title of the PR to account for this.

@gakonst gakonst changed the title Update serde Update serde + dynamically fetch XRP credentials Aug 29, 2019

@gakonst
Copy link
Member

left a comment

LGTM, thanks for adding the feature for dynamic fetch of XRPL credentials. I modified the PR title to include that for clarity.

@gakonst gakonst merged commit 35a57f0 into master Aug 29, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@gakonst gakonst deleted the es-update-serde branch Aug 29, 2019


#[allow(unused)]
pub fn get_xrp_credentials() -> XrpCredentials {
// TODO make this async

This comment has been minimized.

Copy link
@gakonst

gakonst Aug 29, 2019

Member

I will do this in some follow up PR involving the engine tests.

@emschwartz

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

@gakonst In the future, I would not squash the commits for a PR like this. Each commit adds something different and it would be better to retain that in the commit history.

Squashing commits is mostly useful when we work on adding features and have a lot of subcommits that are all basically adding pieces of that one feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.