-
Notifications
You must be signed in to change notification settings - Fork 8
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
API Proposal for Connecting Managed Player to FuelTank #12
Comments
Hello @gormaaen, there is no transaction associated to "CreateWallet" as that's not something that happens in the blockchain itself but rather outside of it and thus there is no way to return a transaction as suggested on solution 2. Personally I don't see much difference between the things of what you proposed and simply check if the address was created already or not. Here is a full case example:
About solution 1, if your proposal was to hold the query until it was created that could lead to a timeout. If your solution is supposed to return false/false/false until it gets create and return true... That's exactly the same thing as the previous suggestion. |
Hi Leonardo, I get the polling requirements, but it is still not intuitive and I am sure alot of devs will fall into the trap of believing the Wallet is fully formed when the system returns true. Two reasons why returning a Transaction is preferred is that 1. it force devs to either deploy a polling sequence and/or 2. allows for the use of the PusherClient to catch the Transaction there. If you deside to stay with the current implementation then maybe just add a comment to the inline comments for SendCreateWallet? Cheers, |
Returning a transaction is not an option. The transactions are on-chain transactions, they have transactionId, hash, events, encoded data, etc.... None of these are available in Creating a Wallet as it is not a transaction. We can't just include something that is not a transaction in there. There are workers/processors/parsers lots of stuff that expect a transaction in the transactions table. But we can try to think about something else. I will raise your concern and check what we can do |
@leonardocustodio Isn't it possible to use an idempotency key for this? It's one of the uses they can be used for. That way you can issue the same transaction again, with the same key, and if it wasn't processed, it'll be processed, but otherwise it'll return the result that wasn't returned last time it was called. It's going to be an issue anyway if the call fails due to some network issue and you can't get any details to query anyway, so providing the unique idempotency key ourselves, allows us to recover in the case that the results were never returned too. |
I see so your suggestion would be: Call @gormaaen do you think that would be a good solution for your use-case? If so @CliffCawley is right and we can easily implement that pretty quick. |
Hi guys,
I like the proposed solution (idempotency key) better than the current
solution, since we will only get a Wallet back when it is fully formed. Of
course we still need a seperate polling cycle. The ideal solution would
also trigger an Event for the Wallet Created event, for those who use the
Pusher Client...but this is more a nice-to-have.
Cheers,
tor. 26. okt. 2023 16.38 skrev Leonardo Custodio ***@***.***>:
… I see so your suggestion would be:
Call Create Wallet the first time, get the idempotency key and keep
calling Create Wallet with the idempotency key until it returns the
"Wallet" (address), is that correct? Or obviously, you guys provide the
idempotency key the first time and just keep calling it until it returns
the "wallet".
@gormaaen <https://github.com/gormaaen> do you think that would be a good
solution for your use-case? If so @CliffCawley
<https://github.com/CliffCawley> is right and we can easily implement
that pretty quick.
—
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AYZ5QHKXVE5V4HWB7LCK6VDYBJYXFAVCNFSM6AAAAAA5XRG4WSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBRGI3DGNRXHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@leonardocustodio @gormaaen I'm not sure if it's good practice to call it an idempotency key or not, but the functionality is similar. So just quickly, before it's called this, just double check what you think. The definition of idempotency is Additionally So really, if the result changes over time, it's probably not the true meaning (I.e. if it's changing from first call 'In progress' to successive calls eventually changing to 'Success') then it's possible it might cause confusion for some developers if it's called that name. Or maybe this is fine. But the mechanic is similar. A client of the API calls with a unique key (Could even be called request key?). Either way, it must be unique, in that it's never been used for another transaction. And so (I copied below from square dev documentation):
Idempotency keys can be anything, but they need to be unique. Virtually all popular programming languages provide a function for generating unique strings. You should use one of these language calls. Anyway, up to you, but if we supply a unique key, and you keep track of those keys to requests, double checking the params and the key are the same and following the rules above, then I think it should work well. If storage of keys/requests is too much, they could potentially expire over time and be deleted, but that wouldn't be standard practice for idempotency keys I don't think. |
If we consider the whole concept I guess it cannot be called a idempotency key. But the alternatives to that would be:
Or something like this https://restfulapi.net/rest-api-design-for-long-running-tasks/ that is not possible on GraphQL as we don't make use of the status codes. Either way, I don't see how that would change anything from the current behavior (getting a status and then querying another place until it returns the data). It is pretty much the samething. What I can suggest in here so we don't make a confusion for other developers is we keep using |
Also just to note this query is already "idempotent" in a way that a That happens because the |
No, that's where the issue is, it needs to be generated by the client, not the server, so that if something happens to the call on the way to the server (network issue, whatever), we can call the same thing again, with the same id, and you'll either process it the first time, or just return the results of the previous call. That's what changes it from the current behaviour. It's kind of like and idempotency key, but we're then also using it to get the progress of a long running process. It's kind of like combining an idempotency key together with a returned jobid, except generated by the caller. But yes, if passing externalId multiple times to this, results in the same result, without creating duplicate entries of anything, then yes it's already idempotent. |
Just to let you guys know this change needs to be done first on |
The issue with the current implementation is that SendCreateWallet will return true even if the resulting player wallet as no Address assigned. This means that expected workflow for creating a managed player and immediately assigning them to a FuelTank will fail (SendCreateWallet .success -> GetWallet.success -> AddAccount using returned wallet.Account.Address) If you add a wait 3secs or so between succesfull SendCreateWallet and GetWallet you might get lucky and have the Address field field filled out. This not ideal.
Solution 1. SendCreateWallet only returns true when Wallet has been completely formed (with Address and Public Key).
Solution 2. SendCreateWallet returns a Transaction for the Wallet creation (with Address and Public Key).
Since creating a managed player and adding a FuelTank ref is expected to be the most common scenario why not do both in one call
Solution 3. SendCreateWallet.SetTankId().SetExternalId() for a combined create managed wallet + add fueltank. Returning a Transaction.
The text was updated successfully, but these errors were encountered: