Closed drkatz closed 5 years ago
Some additional comments:
Let's allow the user to look up by the chain id as well as the token-id + issuer-id.
It might be nice to be able to look up, "what are my non-zero balances for this address for any FAT tokens fatd knows about" so a query across all tokens.
Im not sure we should have an API call for deleting a token's data. Since a token is stored in a database, the user can just remove the database. maybe we add a blacklist/whitelist feature to stop fatd from tracking certain tokens. I don't see a strong need to give this much control to an API.
Awesome work on the error codes and the initial spec overall. Really solid. I may need to rework my jsonrpc lib to allow those error codes through. But it probably should if it doesn't... Need to double check that...
Thanks for the comments @AdamSLevy, will address these in a upcoming commit.
@AdamSLevy The above commit 3bad877:
delete-token
and add chain-id
)token-id
& issuer-id
OR chain-id
For now I'll hold out on the method to get nonzero balances for an address among tracked tokens, as we should strategize a bit. How expensive do you envision this being? Off the top of my head, given a list of addresses it's pretty efficient to check the balance for each token (It's a key value relationship). What are your thoughts?
I think the spec should note just to be explicit, that specifying chain-id
is required to be either the tx or issuance chain. Either is fine since we're just trying to find the token ID & Issuer from ExtIds and either will provide that information.
Warning beforehand. I started at the RPC spec, so some questions/remarks might be of ;)
get-issuance:
- in the request you have a issuer-id whilst in the response it is "issuer"
- why is token-id not returned (I know you supply it already)?
get-transaction:
- Is there are reason I have to provide the token-id and issuer-id here? The tx-id is globally unique right? It is mandatory for almost all other methods, so that might be a symmetry reason
- tx-id vs txId
get-transactions:
- fa-address: Input or output or both?
- why can't I control pagination myself?
delete-token: Although only local, don't you want to sign this somehow?
get-daemon-tokens vs delete-token Daemon in one but not in other
A HTTP 400 code for invalid address is not what I would expect from a REST perspective. I would expect another 404 (depends a bit on what you define as invalid address though) Because at least you also want an address not found
I would suggest HTTP 409 for Token Syncing. 408 might mean people actually think you want to close a connection and expect corresponding header fields ;)
in the request you have a issuer-id whilst in the response it is "issuer"
Actually "issuer' isn't even returned in the response according to the PR that was merged to master earlier today(#15). The library implementing the RPC should splice in the issuer root chain ID if desired.
why is token-id not returned (I know you supply it already)?
Because it already has to be known to make the call in the first place :wink:
Is there are reason I have to provide the token-id and issuer-id here?
Search space efficiency & database structure. The database on the FAT Daemon is structured such that each FAT token has its own database, and it's own database file as a consequence. This database separation allows easier debugging and portability, and gets around column modularity(this is sqlite not nosql). As a consequence, the number of transactions to be queried over is lower since the tx tables are separated, but you need to supply token-id and issuer-id.
tx-id vs txId
:potato: :tomato: I'm open to renaming this thing, but then we'd need to rename lots of other things :wink:
fa-address: Input or output or both?
Both. Thanks for pointing this out, the spec should be more specific. Perhaps in the future we can allow choosing.
why can't I control pagination myself?
Could you please explain how you are limited in your control of pagination with the current API?
[delete-token] Although only local, don't you want to sign this somehow?
delete-token
was removed from the standard in the last commit
Daemon in one but not in other
delete-token
was removed, but the reason for get-daemon-tokens over get-tokens is that get-tokens is ambiguous with regards to other functionality in the API.
A HTTP 400 code for invalid address is not what I would expect from a REST perspective.
This means the factoid address submitted with fa-address
was malformed, or 400 - bad request(malformed request). Should this be clarified in the spec better? 404 doesn't quite make sense for these purposes. A "not found"(unused) valid Factoid address in FAT has a balance of 0, just like in Factom.
I would suggest HTTP 409 for Token Syncing
Totally right, 409 is a better fit for this case for a myriad of reasons. Thanks!
408 implies some "turning of the gears" and giving up after sometime, however a response in the case of syncing would return almost immediately.
Because it already has to be known to make the call in the first place 😉
Yes, but if you are doing a lot of async processing having the correlation in the response itself is handy ;)
why can't I control pagination myself? Could you please explain how you are limited in your control of pagination with the current API?
I would like to specify the amount of pagination but was probably looking at an older version ;)
BTW personaly I "hate" it when an API has multiple names for the same params/responses. tx-id vs txId :)
@nklomp I would argue that any JSON RPC 2.0 client that abides by request IDs would already correlate the request with the response. No?
I "hate" it when an API has multiple names for the same params/responses
I'm not a fan either, we'll do our best :wink: . The responses are more or less fixed in the returned fields due to the standards they retrieve data for, but the RPC params are flexible right now
@nklomp I would argue that any JSON RPC 2.0 client that abides by request IDs would already correlate the request with the response. No?
True, but still means I have to keep track of the token-ids. Just a matter of taste I guess. If you do mass API calls I find it nice to get the params you supplied back (when it makes sense).
Regarding error code -32801 https://github.com/Factom-Asset-Tokens/FAT/blob/FATIP-300-FAT-RPC-API-Standard/fatips/300.md#-32801---invalid-token
How would this "invalid issuance" situation occur? At the moment the spec for fat-0 allows for multiple invalid issuance to occur prior to the single accepted issuance entry. My implementation simply ignored invalid issuance entries. So is this a token chain that exists and is tracked but is not yet issued?
Regarding error code -32801 https://github.com/Factom-Asset-Tokens/FAT/blob/FATIP-300-FAT-RPC-API-Standard/fatips/300.md#-32801---invalid-token
How would this "invalid issuance" situation occur? At the moment the spec for fat-0 allows for multiple invalid issuance to occur prior to the single accepted issuance entry. My implementation simply ignored invalid issuance entries. So is this a token chain that exists and is tracked but is not yet issued?
I think the invalid issuance would occurr if the token chain had been established, but did not contain a valid issuance(s)
There is an issue with send-transaction that I see. When you submit a signature I believe it is best practice to submit the raw signed data as well. So rather than submit the valid tx JSON, the user should instead provide the raw hex data that represents the exact JSON structure signed by the signatures.
If someone wants to encode a tx with extra whitespace that is fine but we need the exact data to verify the signature.
On the backend this data will get converted to the raw bytes the hex represents, and then unmarshalled into the concrete type and checked for validity, all prior to submitting the entry with the exact content data the user supplied.
There is an issue with send-transaction that I see. When you submit a signature I believe it is best practice to submit the raw signed data as well. So rather than submit the valid tx JSON, the user should instead provide the raw hex data that represents the exact JSON structure signed by the signatures.
If someone wants to encode a tx with extra whitespace that is fine but we need the exact data to verify the signature.
On the backend this data will get converted to the raw bytes the hex represents, and then unmarshalled into the concrete type and checked for validity, all prior to submitting the entry with the exact content data the user supplied.
Have we already debated the option of fatd integrating with walletd and handling the signing? (the same way walletd do the signing when using methods such as compose-entry
). Walletd can hold EC keys, FCT keys and even identity keys, all of which we need when dealing with FAT tokens, so having fatd work with walletd doesn't sound too far fetched.
Otherwise in the current situation the client of the daemon that want to call send-transaction
is almost building all the entry itself and then send-transaction
just forward it to factomd, with some extra validation on top.
NOTICE This standard has been deprecated in favor of fatd specifying it's own RPC documentation. The RPC API will not be standardized, and left up to each implementation.
Initial standard defines the following methods:
Token get
Token write
Daemon get
RPC Error codes