aeternity / aepp-calldata-js

Aeternity data serialization library
ISC License
3 stars 4 forks source link

Accept also a contract id for the address type #211

Closed jyeshe closed 1 year ago

jyeshe commented 1 year ago

There is a common flow on NFT marketplaces to allow a contract to operate the tradings on your behalf. For that, there is the approve entrypoint on the AEX-141 standard.

However, when called with a contract id it gives the error:

/home/rogerio/Workspace/ae/aex141-nft-collection-example/node_modules/@aeternity/aepp-calldata/src/ApiEncoder.js:82
            throw new FateTypeError(
                  ^
FateTypeError: Account pubkey should start with ak_, got ct_c3WzV2Ct4Q7PWLrBpAbzHeGbdwWuSnGN83RNwd99zJ538gCWp instead
    at ApiEncoder.decodeWithType (/home/rogerio/Workspace/ae/aex141-nft-collection-example/node_modules/@aeternity/aepp-calldata/src/ApiEncoder.js:82:19)

With ak_ it works but could the lib convert it?

dincho commented 1 year ago

@davidyuk @marc0olo @hanssv @thepiwo do you think this is feasible/good idea?

thepiwo commented 1 year ago

I think the user should manually convert it, the sdk could provide a util to do so

dincho commented 1 year ago

Yeah, my opinion is similar, otherwise we kinda defeat the account types purpose

hanssv commented 1 year ago

The types are there for a reason a don't confuse a spoon for a fork 😅

davidyuk commented 1 year ago

I know that ak to ct can't be converted automatically because the contract in Sophia needs to have some interface. But can't we use ct converted to ak anywhere where the usual account address is used? I would assume that ContractAddress extends AccountAddress and by "polymorphism" it can be used as AccountAddress (a: address), but not the other way.

the sdk could provide a util to do so

The shortest that I have now is

encode(decode(ctAddress), Encoding.AccountAddress)

maybe ctAddress.replace('ct_', 'ak_') would work the best 😃

marc0olo commented 1 year ago

yeah we had that discussion a few times already in the past I think 😅 it's more an educational topic IMO

dincho commented 1 year ago

Alright, thanks all. It is clear that this won't be implemented in a implicit way in this library.