Conversation
dessaya
force-pushed
the
remove-nft-stuff
branch
from
e3e7df4 to
ffdb58f
Compare
|
|
||
| type GetBalanceRequest struct { | ||
| Owner *iotago.Address | ||
| CoinType iotago.ObjectType // optional |
There was a problem hiding this comment.
Why was this ObjectType removed?
Wouldn't it make sense to keep a specific type for CoinTypes coming from the API, just to indicate it properly?
There was a problem hiding this comment.
I replaced the old type ObjectType = string with a more strict ObjectType struct {s string} to avoid subtle bugs where the string contains an invalid or non-canonical format.
Here I just replaced the argument type with string, so it keeps the functionality. I didn't remove it.
| return err | ||
| } | ||
| // use ShortString to save space | ||
| e.WriteString(rt.ShortString()) |
There was a problem hiding this comment.
In the beginning of our first Alpha we had an issue that we stored CoinTypes as ShortString but compared them with FullName Type strings, which caused several problems in the accounting contract. I'm not sure what the actual solution was. If we do a length agnostic check, or if we always use short/full length strings.
Just want to point out that we should validate this carefully, so we don't sometimes store full and short length types, or compare them incorrectly.
There was a problem hiding this comment.
Yes, that's why I defined other related types as struct {s string} instead of just string. For now let's keep it like this (if the tests pass) and when we get time to refactor properly let's unify the types and make sure this bug cannot happen.
| ObjectID: &data.ObjectID, | ||
| Options: &iotajsonrpc.IotaObjectDataOptions{ShowContent: true}, | ||
| }) | ||
| // for coins the "field name" is of type 0x1::ascii::String |
There was a problem hiding this comment.
Is this really the case for only coins and nothing else?
There was a problem hiding this comment.
Yes, the key of the coins in the assets bag is the cointype converted to string, and for objects the key is the object ID.
🤷♂️
| argB := iotago.Argument{NestedResult: &iotago.NestedResult{Cmd: *argBorrow.Result, Result: 1}} | ||
|
|
||
| for coinType, coinBalance := range assets.Coins { | ||
| for _, coinType := range slices.Sorted(slices.Values(maps.Keys(assets.Coins))) { |
There was a problem hiding this comment.
why do we need to sort the coins here?
There was a problem hiding this comment.
It's to make sure that the resulting PTB is deterministic.
Probably it's not needed, but I'm not sure.
| Module: iscmove.AssetsBagModuleName, | ||
| Function: "take_asset", | ||
| TypeArguments: []iotago.TypeTag{assetsBag.Objects[id].TypeTag()}, | ||
| Arguments: []iotago.Argument{argAssetsBag, ptb.MustPure(id)}, |
There was a problem hiding this comment.
If the object is deposit and withdraw in the same PTB, then ptb.MustPure(id) may cause error, because the same id will show up more than once.
There was a problem hiding this comment.
Ok, should I replace it with ptb.MustForceSeparatePure ?
3 participants
No description provided.