Model validation #39

CreatureDev · 2023-08-21T13:16:45Z

No description provided.

model/client/admin/server/ledger_accept_request.go

bmurphy333 · 2023-10-26T22:25:19Z

Need to add the limit check (10 < L < 400) on all requests with limit as param. Could move the logic from account_lines into a helped method and call that in all places?

CreatureDev · 2023-10-26T22:32:32Z

Need to add the limit check (10 < L < 400) on all requests with limit as param. Could move the logic from account_lines into a helped method and call that in all places?

Not all functions that have a limit have the same bounds, e.g. crawl shards 1<= l <= 3, account nfts 20 <= l <= 400

Functions that take a limit also explicitly state that invalid values are changed to the closest valid value. Is it really necessary to validate the values when the ledger will accept and correct invalid values?

bmurphy333 · 2023-10-26T22:36:55Z

Ahh fairs if they will be different for others. It's just that some with the same bounds have been validated and some not (e.g. account_channels and account_lines). Unless i'm mistaken. Does account_lines not need that limit validation then?

CreatureDev · 2023-10-26T22:40:30Z

I don't know of any method where an invalid limit will return with an error
I was originally validating on them, but changed my mind part way through implementation on whether it should be validated
We can make a decision to include them and I will add it to the rest of the methods, or we can remove limit validation entirely

joshwalong · 2023-10-26T23:32:38Z

model/client/clio/nft_info_request.go

do we need to check for NFTokenID not empty?

joshwalong · 2023-10-26T23:37:59Z

model/client/server/manifest_request.go

check for pub key not empty and potentially that it's base58 encoded?

joshwalong · 2023-10-26T23:43:41Z

model/client/channel/channel_authorize_request.go

Channel id is 64 char hex string, worth checking for the validate?
XRP amount looks like a required parameter.
also a check for “The request must specify exactly one of secret, seed, seed_hex, or passphrase.”

I implemented XRPAmount as an integer, so if it is not set it would be 0 which would be valid

joshwalong · 2023-10-26T23:47:36Z

model/client/channel/channel_verify_request.go

Looks like these are required:

XRP Amount

Channel id (64 char hex string)

Public key (hex or base58 string)

Signature (hex string)

XRP Amount same as above, 0 is valid

joshwalong · 2023-10-26T23:52:40Z

model/client/path/ripple_path_find_request.go

should we check if ledger_hash is provided that it's a 20 byte hex string?

bmurphy333 · 2023-10-26T22:09:03Z

model/client/account/account_channels_request.go

-	if a.Account == "" {
-		return errors.New("no account ID specified")
+func (r *AccountChannelsRequest) Validate() error {
+	if err := r.Account.Validate(); err != nil {


in the validate method there is still a
// TODO checksum
does that need adding still?

yeah, I have not added any intricacies for checking valid values for thinks like addresses, keys or object IDs with encoded fields

bmurphy333 · 2023-10-26T22:26:27Z

model/client/account/account_lines_request.go

@@ -20,6 +21,17 @@ func (*AccountLinesRequest) Method() string {
 	return "account_lines"
 }

+func (r *AccountLinesRequest) Validate() error {


Need to also run the account.Validate on the 'peer' parameter if it is present

bmurphy333 · 2023-10-26T23:20:18Z

model/client/ledger/ledger_entry_request.go

+
+	return nil
+}
+


Do we need to add the 'amm' and 'nft_page' options too? They're not in the request struct so may have been recently added

AMM does not appear to be getting approved too soon
we can do AMM support once it is reaching final stages of approval into rippled

bmurphy333 · 2023-10-26T23:26:06Z

model/client/ledger/ledger_entry_request.go

+
+	if r.RippleState != nil {
+		setCount++
+		if err := r.Offer.Validate(); err != nil {


should this be r.rippleState.Validate() ?

bmurphy333 · 2023-10-26T23:37:04Z

model/client/ledger/ledger_entry_request.go

@@ -37,6 +118,10 @@ type DirectoryEntryReq struct {
 	Owner    string `json:"owner,omitempty"`
 }

+func (*DirectoryEntryReq) Validate() error {
+	return nil
+}


should this have the logic for: requires either dir_root or owner as a sub-field

bmurphy333 · 2023-10-26T23:41:41Z

model/client/ledger/ledger_entry_request.go

 type TicketEntryReq struct {
 	Account   types.Address `json:"account"`
 	TicketSeq int           `json:"ticket_seq"`
 }

 func (*TicketEntryReq) LedgerEntryRequestField() {}

+func (r *TicketEntryReq) Validate() error {


Number also required here I think

bmurphy333 · 2023-10-26T23:44:51Z

model/client/transactions/tx_request.go

@@ -12,3 +12,7 @@ type TxRequest struct {
 func (*TxRequest) Method() string {
 	return "tx"
 }
+
+func (*TxRequest) Validate() error {
+	return nil


Think we need to validate transaction is not empty here?

bmurphy333 · 2023-10-26T23:46:12Z

model/client/transactions/transaction_entry_request.go

@@ -38,3 +38,7 @@ func (t *TransactionEntryRequest) UnmarshalJSON(data []byte) error {
 	t.LedgerIndex = i
 	return nil
 }
+
+func (*TransactionEntryRequest) Validate() error {
+	return nil


Again, do we not need to validate the tx_hash is not empty here?

bmurphy333 · 2023-10-26T23:47:39Z

model/client/transactions/submit_multisigned_request.go

@@ -35,3 +35,7 @@ func (r *SubmitMultisignedRequest) UnmarshalJSON(data []byte) error {

 	return nil
 }
+
+func (*SubmitMultisignedRequest) Validate() error {
+	return nil


Need to validate on the tx_json as it's required. Think just a nil check would be ok here without getting into the json itself?

bmurphy333 · 2023-10-27T00:03:41Z

model/client/transactions/submit_request.go

@@ -8,3 +8,7 @@ type SubmitRequest struct {
 func (*SubmitRequest) Method() string {
 	return "submit"
 }
+
+func (*SubmitRequest) Validate() error {
+	return nil


Need to check tx_blob as it's required

Ohhhh I've just thought - are these going to be validated differently? We need to build out our submit methods but these will build out the tx_blob anyway, so won't need to validate on it?

We can have this request constructed by a helper method that would validate the transaction during construction
This request should probably still validate that the blob is at least set

bmurphy333 · 2023-10-27T01:07:57Z

model/client/admin/key/wallet_propose_request.go

@@ -10,3 +10,7 @@ type WalletProposeRequest struct {
 func (*WalletProposeRequest) Method() string {
 	return "wallet_propose"
 }
+
+func (*WalletProposeRequest) Validate() error {
+	return nil


key_type is a required param
Need to add the logic for:
You must provide at most one of the following fields: passphrase, seed, or seed_hex
So cannot have more than one of them.

Or ir does say you can run it with no params to create a random seed, so maybe don't need the required check on key_type

bmurphy333 · 2023-10-27T01:12:46Z

model/client/admin/data/download_shard_request.go

@@ -14,3 +14,7 @@ type ShardDescriptor struct {
 func (*DownloadShardRequest) Method() string {
 	return "download_shard"
 }
+
+func (*DownloadShardRequest) Validate() error {


should we assert on shards not being nil? Bit stupid if user does send empty tho

bmurphy333 · 2023-10-27T01:18:18Z

model/client/admin/peer/connect_request.go

@@ -8,3 +8,7 @@ type ConnectRequest struct {
 func (*ConnectRequest) Method() string {
 	return "connect"
 }
+
+func (*ConnectRequest) Validate() error {
+	return nil


ip is a required param

bmurphy333 · 2023-10-27T01:19:39Z

model/client/admin/peer/peer_reservations_add_request.go

@@ -8,3 +8,7 @@ type PeerReservationAddRequest struct {
 func (*PeerReservationAddRequest) Method() string {
 	return "peer_reservations_add"
 }
+
+func (*PeerReservationAddRequest) Validate() error {
+	return nil


public_key is a required field. Also should be base58 encoded, so should we do a check on that? Not sure yet how will talk to Josh

bmurphy333 · 2023-10-27T01:20:17Z

model/client/admin/peer/peer_reservations_del_request.go

@@ -7,3 +7,7 @@ type PeerReservationDelRequest struct {
 func (*PeerReservationDelRequest) Method() string {
 	return "peer_reservations_del"
 }
+
+func (*PeerReservationDelRequest) Validate() error {


Again need to think how we do a check on the public_key

bmurphy333 · 2023-10-27T01:22:35Z

model/client/admin/status/get_counts_request.go

@@ -7,3 +7,7 @@ type GetCountsRequest struct {
 func (*GetCountsRequest) Method() string {
 	return "get_counts"
 }
+
+func (*GetCountsRequest) Validate() error {


not sure if min_count is really required or not

joshwalong · 2023-10-27T04:01:11Z

model/client/admin/signing/sign_request.go

Does this cover these cases?:
Provide the seed in the secret field and omit the key_type field. This value can be formatted as an XRP Ledger [base58](https://xrpl.org/base58-encodings.html) seed, RFC-1751, hexadecimal, or as a string passphrase. (secp256k1 keys only) Provide a key_type value and exactly one of seed, seed_hex, or passphrase. Omit the secret field. (Not supported by the commandline syntax.)

I currently don't do any validation of string format

joshwalong · 2023-10-27T04:14:21Z

model/client/admin/signing/sign_for_request.go

Fee and Sequence must be provided in the tx_json object and also signingpubkey must be provided with an empty string as the value. Can we do this in validate func?

Also below is similar to sign_request:
`You must provide exactly 1 field with the secret key, which can be either of the following:

Provide a secret value and omit the key_type field. This value can be formatted as an XRP Ledger base58 seed, RFC-1751, hexadecimal, or as a string passphrase. (secp256k1 keys only)
Provide a key_type value and exactly one of seed, seed_hex, or passphrase. Omit the secret field. (Not supported by the commandline syntax.)`

CreatureDev marked this pull request as ready for review October 5, 2023 19:24

CreatureDev added 12 commits October 5, 2023 12:25

account validation

f4d4f1b

admin validation

294baa6

improve error

f3c5352

channel validation

651a7e4

clio validation

ec0b614

ledger request validation

efbcd0d

path/book validation

1480b93

server request validation

435b6c8

subscribe validation

4e762f2

transactions validation

b762889

utility validation

5c5a725

addl tests

22d44cb

CreatureDev force-pushed the pr/model-validation branch from 1f4aedd to 22d44cb Compare October 5, 2023 19:25

CreatureDev requested review from bmurphy333, joshwalong and JCrawsh October 5, 2023 19:26

CreatureDev self-assigned this Oct 5, 2023

joshwalong reviewed Oct 13, 2023

View reviewed changes

model/client/admin/server/ledger_accept_request.go Outdated Show resolved Hide resolved

typo

d8364f0

CreatureDev requested a review from joshwalong October 26, 2023 21:21

joshwalong reviewed Oct 26, 2023

View reviewed changes

model/client/clio/nft_info_request.go Outdated

Copy link

Contributor

joshwalong Oct 26, 2023

Choose a reason for hiding this comment

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

do we need to check for NFTokenID not empty?

joshwalong reviewed Oct 26, 2023

View reviewed changes

model/client/server/manifest_request.go Outdated

Copy link

Contributor

joshwalong Oct 26, 2023

Choose a reason for hiding this comment

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

check for pub key not empty and potentially that it's base58 encoded?

joshwalong reviewed Oct 26, 2023

View reviewed changes

bmurphy333 reviewed Oct 27, 2023

View reviewed changes

joshwalong reviewed Oct 27, 2023

View reviewed changes

requested changes

7c9fd4f

CreatureDev requested review from bmurphy333 and joshwalong January 18, 2024 19:56

Model validation #39

Are you sure you want to change the base?

Model validation #39

Conversation

CreatureDev commented Aug 21, 2023

bmurphy333 commented Oct 26, 2023

CreatureDev commented Oct 26, 2023

bmurphy333 commented Oct 26, 2023

CreatureDev commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment