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.

Sign up for GitHub

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

Jump to bottom

Add `bind_redeem_address` to CLI #3908

Merged

kylezs merged 4 commits into main from feat/cli-bind-redeem-address

Aug 31, 2023

Contributor

j4m1ef0rd commented Aug 29, 2023

Pull Request

Closes: PRO-436

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have updated documentation where appropriate.

Summary

Added bind_redeem_address to the CLI and Operator API
Added get_bound_redeem_address to the CLI and Query API
Changed the redeem command in the CLI to use the bound redeem address if the user does not supply an address.


          feat: added bind command to cli

3fcf509

j4m1ef0rd added the CLI / API label

j4m1ef0rd self-assigned this

linear bot commented Aug 29, 2023

PRO-436 Redeem address binding through the Cli

See parent issue for details.

We basically just want to submit a bind_redeem_address extrinsic via the CLI.

We need to expose this functionality through the CLI.

j4m1ef0rd requested a review from dandanlen

August 29, 2023 05:35


          feat: query to get redeem address

e271bab

j4m1ef0rd force-pushed the feat/cli-bind-redeem-address branch from aa1a162 to e271bab Compare

August 29, 2023 05:41

codecov bot commented Aug 29, 2023 •

edited

Loading

Codecov Report

Merging #3908 (10a164a) into main (7911f33) will decrease coverage by 0%.
Report is 7 commits behind head on main.
The diff coverage is 0%.

@@          Coverage Diff          @@
##            main   #3908   +/-   ##
=====================================
- Coverage     71%     71%   -0%     
=====================================
  Files        366     366           
  Lines      56762   56798   +36     
  Branches   56762   56798   +36     
=====================================
- Hits       40540   40539    -1     
- Misses     14213   14244   +31     
- Partials    2009    2015    +6

Files Changed	Coverage Δ
api/lib/src/lib.rs	`26% <0%> (-1%)`	⬇️
api/lib/src/queries.rs	`0% <0%> (ø)`
state-chain/pallets/cf-funding/src/lib.rs	`69% <ø> (ø)`

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kylezs requested review from kylezs and removed request for dandanlen

August 29, 2023 06:17

kylezs reviewed

View reviewed changes

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved

api/bin/chainflip-cli/src/main.rs Outdated

-              			.context("Invalid ETH address supplied")?
-              			.as_slice(),
-              	);
+              	let eth_address = if let Some(address) = eth_address {

Contributor

kylezs Aug 29, 2023

We should check the bound address first right? If there's a bound address then the eth_address supplied doesn't matter. Then, rather than pushing the redemption through to an address other than the user specified, we should compare the provided address, if provided, and check if they match - providing appropriate messages to the user if it does /doesn't - and exiting if they don't match.

Contributor Author

j4m1ef0rd Aug 29, 2023

Yeah i thought about this and was not sure what approach to take. Thinking about it again, catching the mistake early so the user doesn't waste gas makes more sense.

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved

state-chain/pallets/cf-funding/src/lib.rs Outdated Show resolved Hide resolved

api/bin/chainflip-cli/src/settings.rs Outdated Show resolved Hide resolved

dandanlen reviewed

View reviewed changes

api/bin/chainflip-cli/src/main.rs Outdated

+              async fn get_bound_redeem_address(api: QueryApi) -> Result<()> {
+              	if let Some(bound_address) = api.get_bound_redeem_address(None, None).await? {
+              		println!("Your account is bound to redeem address: 0x{}", hex::encode(bound_address));

Collaborator

dandanlen Aug 29, 2023

EthereumAddress prints as hex by default, no need for explicit conversions. I ran a test:

#[test]
fn eth_address() {
	let e = EthereumAddress::repeat_byte(0xff);
	println!("{} / {:?} / {:x?}", e, e, e);
}

Prints:

0xffff…ffff / 0xffffffffffffffffffffffffffffffffffffffff / 0xffffffffffffffffffffffffffffffffffffffff

Contributor Author

j4m1ef0rd Aug 30, 2023

I was only encoding it manually to avoid it displaying as a shortened string. But the :? does the same, so ill use that.

dandanlen reviewed

View reviewed changes

api/lib/src/queries.rs Outdated Show resolved Hide resolved


          refactor: address PR comments

7beb181

kylezs approved these changes

View reviewed changes

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved

api/bin/chainflip-cli/src/main.rs Outdated Show resolved Hide resolved


          chore: small changes from PR comments

10a164a

kylezs approved these changes

View reviewed changes

kylezs merged commit 168135f into main

42 checks passed

kylezs deleted the feat/cli-bind-redeem-address branch

August 31, 2023 06:35

syan095 added a commit that referenced this pull request


          Merge remote-tracking branch 'origin/main' into feature/ccm-gas-limit

00239ea

* origin/main:
  Fix: Correct Select Median Implementation (#3934)
  fix: independent witnessing startup (#3913)
  🍒 cherry-pick: changes in release for CI and chainspec (#3933)
  refactor: Re-arrange Chains traits for better composability (#3912)
  fix: log error when we try to transfer *more* than we have fetched (#3930)
  chore: add checks and increase timeout (#3928)
  Add `bind_redeem_address` to CLI (#3908)
  🍒 cherry-pick: add missing prod dockerfiles (#3926)
  chore: skip localnet specific tests in devnet 🤫 (#3919)
  fix: broadcast success should be witnessable after a rotation (#3921)

# Conflicts:
#	state-chain/chains/src/eth/api.rs
#	state-chain/runtime/src/chainflip.rs

syan095 added a commit that referenced this pull request


          Merge remote-tracking branch 'origin/main' into test/authority-rotati…

a872d75

…on-integration

* origin/main:
  Added CFE setting for logging span lifecycles (#3936)
  fix: only burn flip if non zero (#3932)
  Fix: Correct Select Median Implementation (#3934)
  fix: independent witnessing startup (#3913)
  🍒 cherry-pick: changes in release for CI and chainspec (#3933)
  refactor: Re-arrange Chains traits for better composability (#3912)
  fix: log error when we try to transfer *more* than we have fetched (#3930)
  chore: add checks and increase timeout (#3928)
  Add `bind_redeem_address` to CLI (#3908)
  🍒 cherry-pick: add missing prod dockerfiles (#3926)
  chore: skip localnet specific tests in devnet 🤫 (#3919)
  fix: broadcast success should be witnessable after a rotation (#3921)

# Conflicts:
#	state-chain/cf-integration-tests/src/network.rs

dandanlen pushed a commit that referenced this pull request


          Add bind_redeem_address to CLI (#3908)

ad34343

* feat: added bind command to cli

* feat: query to get redeem address

* refactor: address PR comments

* chore: small changes from PR comments

dandanlen pushed a commit that referenced this pull request


          Add bind_redeem_address to CLI (#3908)

0eb18aa

* feat: added bind command to cli

* feat: query to get redeem address

* refactor: address PR comments

* chore: small changes from PR comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels