CLI with ARK Sdk #224
CLI with ARK Sdk #224
Conversation
client/main.go
Outdated
| return printJSON(cfg) | ||
| } | ||
|
|
||
| func onboard(ctx *cli.Context) error { |
There was a problem hiding this comment.
this can be dropped once the pr is synced with master
| }) | ||
| } | ||
|
|
||
| func redeem(ctx *cli.Context) error { |
There was a problem hiding this comment.
if --force flag is set we must call arkSdkClient.UnilateralRedeem
client/main.go
Outdated
| offchainAddr, onchainAddr, err := arkSdkClient.Receive(cntx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return printJSON(map[string]interface{}{ | ||
| "offchain_address": offchainAddr, | ||
| "onchain_address": onchainAddr, | ||
| }) |
There was a problem hiding this comment.
Receive now returns the boarding address as onchain address:
| offchainAddr, onchainAddr, err := arkSdkClient.Receive(cntx) | |
| if err != nil { | |
| return err | |
| } | |
| return printJSON(map[string]interface{}{ | |
| "offchain_address": offchainAddr, | |
| "onchain_address": onchainAddr, | |
| }) | |
| offchainAddr, boardingAddr, err := arkSdkClient.Receive(cntx) | |
| if err != nil { | |
| return err | |
| } | |
| return printJSON(map[string]interface{}{ | |
| "offchain_address": offchainAddr, | |
| "boarding_address": boardingAddr, | |
| }) |
|
|
||
| path = strings.Replace(path, "~", homeDir, 1) | ||
| func decrypt(encrypted, password []byte) ([]byte, error) { |
Looks like they are triggered only if there are changes in /server which is wrong. |
| @@ -278,6 +280,13 @@ func receive(ctx *cli.Context) error { | |||
| } | |||
|
|
|||
| func claim(ctx *cli.Context) error { | |||
| if err := arkSdkClient.Unlock( | |||
There was a problem hiding this comment.
wouldnt be more correct to check is IsUnlocked before trying to unlock an already unlocked SDK? to avoid extra roundtrip
There was a problem hiding this comment.
This is the only way, AFAIK, that SDK loads primary key from memory, in the example itself similar flow is used.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, but that wont help, when u init arksdk, pk is not loaded by default, and only place where pk is set is in Unlock method.
Calling IsLocked is meaningless in this case.
| @@ -59,6 +59,7 @@ func main() { | |||
| return fmt.Errorf("error initializing ark sdk client: %v", err) | |||
| } | |||
| arkSdkClient = sdk | |||
|
|
|||
There was a problem hiding this comment.
Are we sure in using a global variable here? Wouldnt be better to have it inside the context?
There was a problem hiding this comment.
Dont see an issue with this, its one file, all function calls, purpose is just to avoid code repetition by setting up client in each func, so basically before each cmd call setup arksdk client and set global var to func.
| liquidRegTestUrl = getEnv("LIQUID_REGTEST_URL", "http://localhost:3001") | ||
| bitcoinRegTestUrl = getEnv("BITCOIN_REGTEST_URL", "http://localhost:3000") |
There was a problem hiding this comment.
shouldn't we use here viper/urfave directly? Usually ENV are automatically ARK prefixed
There was a problem hiding this comment.
wanted to use quicker solution without 3rd party pkg's since this is only relevant for regtest.
There was a problem hiding this comment.
You can use EnvVars of cli when creating flags, so you have both a flag and an env var https://github.com/sekulicd/ark/blob/52b21c5e687111250b365e89438503e2327ce107/client/main.go#L79
There was a problem hiding this comment.
yeah, but var supportedNetworks is inside sdk, imo this is simplest way to inject diff values for regtest without touching existing arksdk interface, otherwise we would need to pass network and this is not good since network comes from ASP via GetInfo. Instead of passing it via CLI env var i decided to pass it throuugh regtest dockerfile's.
There was a problem hiding this comment.
Also..we used to have an --explorer flag!!
Look at the current doc https://arkdev.info/docs/quick-start/client/#setting-up-alice-and-bobs-clients
There was a problem hiding this comment.
This is not relevant after sdk introduction, as there is no way to pass explorer url when creating instance, as said now explorer is created based on net retreived from ASP with GetInfo RPC.
There was a problem hiding this comment.
This is for sure wrong on the SDK side then. Let's fix it!
The explorer URL SHOULD NOT be mandated by the connected ASP, as is used to fetch blockchain data which is an open protocol. (I guess to check onchain balance? Compute timestamps from blocks?).
It's very likely user of the SDK wants to use their explorer, instead being us forcing them with some well-known explorer URLs.
This updates CLI so that it uses arksdk.
Note that this depends on arksdk covenantless update, once arksdk is updated changes should be simple and applicable in here
This closes #218
@tiero @altafan @louisinger please review.