Add asset flag to rvasps #124
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 15, 2023 09:51
pdeziel
approved these changes
Mar 16, 2023
cmd/rvasp/main.go
Outdated
| &cli.StringFlag{ | ||
| Name: "t, asset-type", | ||
| Usage: "the type of virtual asset", | ||
| Value: "bitcoin", |
There was a problem hiding this comment.
Is the asset type supposed to be a shorthand (e.g. BTC, USD, ETH)?
There was a problem hiding this comment.
The transfer requests I looked at have the whole word. I don't know if the TRISA whitepaper makes a recommendation, that would be something to check into.
proto/rvasp/v1/api.proto
Outdated
| @@ -70,6 +70,7 @@ message TransferRequest { | |||
| string account = 1; // email address or crypto wallet of the account to debit | |||
| string beneficiary = 2; // email address or crypto wallet to look up beneficiary with | |||
| float amount = 3; // amount to transfer to the beneficiary (will be truncated to 2 decimal points) | |||
| string asset_type = 8; // the type of virtual asset for multi-asset chains (optional) | |||
There was a problem hiding this comment.
Can we move this to the bottom for clarity? Also, if we're saying it's an optional field here, should it not have a default value in the CLI?
There was a problem hiding this comment.
I had it after amount to keep it in line with the order in the trisa repo protobuf definition but I'll move it to the bottom. Good catch on the optional tag, I added that before I put the default value in the CLI.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the rvasps to add an asset_type flag to transactions.