algorandfoundation / algokit-utils-ts

MIT License
19 stars 8 forks source link

feat: Deprecate transfer and asset modules #291

Closed robdmoore closed 2 months ago

robdmoore commented 5 months ago

Follows on from #287.

Breaking changes:

Added:

Deprecated the following in favour of AlgorandClient functionality:

joe-p commented 5 months ago

Haven't done a proper review yet, but not sure how I feel about algorand.send/transaction.rekey. On one hand it's nice to have such an important operation be so explicit. On the other hand, I'm worried it might give developers the incorrect impression that a rekey is a special transaction type.

robdmoore commented 5 months ago

No different to opt-in/opt-out imo

robdmoore commented 5 months ago

Also allows for a really explicit approach to such a dangerous but useful operation with a minimal set of parameters with intellisense. we could illustrate in the method description that rekey can also be added to any other transaction by setting the optional rekeyTo value?

joe-p commented 5 months ago

No different to opt-in/opt-out imo

To me the main difference is that there are no negative consequences of a developer thinking optin/out is a seperate tx type. With rekeys, however, if a developer thinks that rekey is a special transaction type and reads code somewhere with a payment transaction or appl, they might be oblivious to the fact that it's rekeying an account.

Essentially I'm nervous it might "train" developers to look for .rekey calls rather than rekeyTo fields

robdmoore commented 5 months ago

Another option is to add it to account manager as a method.

joe-p commented 5 months ago

Another option is to add it to account manager as a method.

Yeah I like that. Makes it more clear that it's abstraction and not a transaction type

robdmoore commented 5 months ago

Cool, done.

Funnily enough it was how I originally wrote it, but then I decided to make it a composer thing so you could get the transaction separate to sending it. I think that's a minor benefit though.

joe-p commented 5 months ago

small nit: I would change rekeyAccount to rekeyAddress. In general as we start to get HD wallet support we should try to avoid using "account" to refer to a single address.

robdmoore commented 4 months ago

why we decided to support a union of string | TransactionSignerAccount for address fields?

It was so that you could pass in the result of something like const alice = algorand.account.random() in i.e. you can pass in alice and not have to do alice.addr i.e. algorand.send.payment({from: alice, to: bob}) reads much nicer than algorand.send.payment({from: alice.addr, to: bob.addr})

We could remove that and just stick with string of course if we felt it was too confusing...

robdmoore commented 4 months ago

Also, I see a lot of changes to the formatting of how we saw "Algos". Personally I think non-plural ALGO is the most common. Ie. "I have 10 ALGO" vs "I have 10 Algos"

It's a tough one because I see it written in different ways everywhere, it's wildly inconsistent. I'm not opposed to just using ALGO though, more than happy with that?

robdmoore commented 4 months ago

Should the bulk functions go in the AssetManager?

I thought about that. Maybe that makes sense? I put in account because it was being done in the context of an account, but AccountManager is a really big class now and you could argue either way, so maybe AssetManager is better?

joe-p commented 4 months ago

It was so that you could pass in the result of something like const alice = algorand.account.random()

This begs the question... why not just have .random return a string? Returning an object with the signer makes sense when we are concerned about legacy interop, but now that we're deprecating legacy utils functions maybe it just makes more sense to return a string? I suppose the question really is where do we expect developers to use the signer function manually rather than using it implicitly through the AlgorandClient

It's a tough one because I see it written in different ways everywhere, it's wildly inconsistent. I'm not opposed to just using ALGO though, more than happy with that?

Yeah I don't have a strong opinion on it but consistency is nice at least within the libs we control. Perhaps we can just do a quick poll in slack to see what everyone prefers and just stick with that everywhere.

I thought about that. Maybe that makes sense? I put in account because it was being done in the context of an account, but AccountManager is a really big class now and you could argue either way, so maybe AssetManager is better?

Yeah I can see the argument either way. I personally leans towards AssetManager but have no objections for AccountManager. Although since AccountManager is already a big class and these are relatively uncommon functions to use, it might make more sense to put under AssetManager just for that reason.

robdmoore commented 4 months ago

I like the simplicity of returning a string, but it feels very wrong from an object oriented / code semantics perspective. ‘String’ is too generic a type for something that important and it would make it much harder to detect when you’re passing the wrong thing in the wrong place maybe?

agree on the other two. Did you want to raise the poll for algos?

joe-p commented 4 months ago

I like the simplicity of returning a string, but it feels very wrong from an object oriented / code semantics perspective. ‘String’ is too generic a type for something that important and it would make it much harder to detect when you’re passing the wrong thing in the wrong place maybe?

I agree but also think that's just the nature of nominal type systems. I'm not convinced it's worth creating an object just for this purpose.

agree on the other two. Did you want to raise the poll for algos?

Just posted in slack 👍

robdmoore commented 3 months ago

Regardless of whether we decide to do string or not, it makes sense to decouple that from this PR since it's a much wider / bigger / unrelated change.

I also didn't change rekeyAccount to rekeyAddress because there are many other things that tie account = single address, that's a broader change that will somehow need to be incorporated into the Algorand ecosystem with thought to avoid confusion.

Otherwise incorporated all the other feedback.