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

CLI binding executor address #4042

Merged
merged 4 commits into from
Sep 25, 2023
Merged

CLI binding executor address #4042

merged 4 commits into from
Sep 25, 2023

Conversation

j4m1ef0rd
Copy link
Contributor

@j4m1ef0rd j4m1ef0rd commented Sep 22, 2023

Pull Request

Closes: PRO-840

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 cmds to the CLI: BindExecutorAddress, GetBoundExecutorAddress.
  • Added a check to the redeem cmd that looks at the bound executor address and uses it or gives a nice error.

@linear
Copy link

linear bot commented Sep 22, 2023

PRO-840 Add executor binding to CLI

The functionality added in the parent issue needs to be added to the CLI.

Should be mostly cut-and-paste based on the redeem address binding.

@j4m1ef0rd j4m1ef0rd removed the request for review from martin-chainflip September 22, 2023 07:26
@@ -165,6 +172,35 @@ async fn request_redemption(
bail!("No redeem address supplied and no bound redeem address found for your account {account_id}."),
};

// Check the bound executor address for this account
let supplied_executor_address = if let Some(address) = supplied_executor_address {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use-case of providing an executor address?

Copy link
Contributor

@kylezs kylezs Sep 22, 2023

Choose a reason for hiding this comment

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

See the original issue: https://linear.app/chainflip/issue/PRO-823/bind-nodes-executor-to-an-address
It's for Liquidity Staking Providers

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

👍

},
#[clap(about = "Restricts your account to only be able to redeem to the specified address")]
BindRedeemAddress {
#[clap(help = "The Ethereum address you wish to bind your account to")]
eth_address: String,
},
#[clap(
about = "Restricts your account to only be able to execute registered redemptions with the specified address"
Copy link
Contributor

Choose a reason for hiding this comment

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

while you're here, I think it would be nice to say that it's irreversible, same for the other binding commands, rather than wait until they start the process to see it, they can see it when they --help

Comment on lines 372 to 374
ensure!(executor.is_some(), Error::<T>::ExecutorBindingRestrictionViolated);
ensure!(
executor_addr == executor.unwrap_or_default(),
executor_addr == executor.unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should avoid the unwrap here:

if let Some(executor_addr) = BoundExecutorAddress::<T>::get(&account_id) {
		let executor = executor.ok_or(Error::<T>::ExecutorBindingRestrictionViolated)?;
		ensure!(executor_addr == executor, Error::<T>::ExecutorBindingRestrictionViolated);
}

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #4042 (397aafc) into main (e94f1f2) will decrease coverage by 0%.
Report is 6 commits behind head on main.
The diff coverage is 3%.

@@          Coverage Diff           @@
##            main   #4042    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        369     369            
  Lines      58591   59302   +711     
  Branches   58591   59302   +711     
======================================
+ Hits       42381   42650   +269     
- Misses     14111   14531   +420     
- Partials    2099    2121    +22     
Files Changed Coverage Δ
api/lib/src/lib.rs 31% <0%> (-1%) ⬇️
api/lib/src/queries.rs 0% <0%> (ø)
state-chain/pallets/cf-funding/src/lib.rs 69% <50%> (-<1%) ⬇️

... and 27 files with indirect coverage changes

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

@j4m1ef0rd j4m1ef0rd merged commit 3e47468 into main Sep 25, 2023
44 checks passed
@j4m1ef0rd j4m1ef0rd deleted the feat/cli-executor-binding branch September 25, 2023 23:41
dandanlen added a commit that referenced this pull request Sep 26, 2023
* feat: cli binding executor address

* refactor: avoid unwrap

* docs: update readme and cmd description

* chore: add example

---------

Co-authored-by: Daniel <daniel@chainflip.io>
dandanlen added a commit that referenced this pull request Sep 26, 2023
* feat: cli binding executor address

* refactor: avoid unwrap

* docs: update readme and cmd description

* chore: add example

---------

Co-authored-by: Daniel <daniel@chainflip.io>
dandanlen added a commit that referenced this pull request Sep 28, 2023
* feat: cli binding executor address

* refactor: avoid unwrap

* docs: update readme and cmd description

* chore: add example

---------

Co-authored-by: Daniel <daniel@chainflip.io>
dandanlen added a commit that referenced this pull request Oct 9, 2023
* feat: cli binding executor address

* refactor: avoid unwrap

* docs: update readme and cmd description

* chore: add example

---------

Co-authored-by: Daniel <daniel@chainflip.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants