aepp-sdk-js icon indicating copy to clipboard operation
aepp-sdk-js copied to clipboard

inconvenient usage of FATE variant types related to AENS

Open marc0olo opened this issue 4 years ago • 13 comments

this issue is closely related to https://github.com/aeternity/aepp-sdk-js/issues/1253 which is now solved and yes, we are able to handle those types now in the SDK.

as @thepiwo recently updated AEproject I took the chance to revisit my old draft PR where I wanted to test all the delegation signature stuff and opened a bunch of issues, see https://github.com/aeternity/aepp-sophia-examples/pull/67

actually I finally succeeded in encoding/decoding AENS relevant stuff (yeah 😎) but I am not really happy in the way we have to deal with the pointers right now:

js test

  it('should add and get pointers correctly', async () => {
      const accountPointerKey = SCHEMA.NAME_ID_KEY['ak'];
      const oraclePointerKey = SCHEMA.NAME_ID_KEY['ok'];
      const contractPointerKey = SCHEMA.NAME_ID_KEY['ct'];
      const channelPointerKey = SCHEMA.NAME_ID_KEY['ch'];
      let getPointersResult = await contract.methods.get_pointers(testName);
      console.log(getPointersResult.decodedResult);
      await contract.methods.add_key(nameOwnerKeypair.publicKey, testName, accountPointerKey, {'AENS.AccountPt':[nameOwnerKeypair.publicKey]}, aensDelegationSignature, { onAccount: delegateKeypair });
      await contract.methods.add_key(nameOwnerKeypair.publicKey, testName, oraclePointerKey, {'AENS.OraclePt':[nameOwnerKeypair.publicKey]}, aensDelegationSignature, { onAccount: delegateKeypair });
      await contract.methods.add_key(nameOwnerKeypair.publicKey, testName, contractPointerKey, {'AENS.ContractPt':[nameOwnerKeypair.publicKey]}, aensDelegationSignature, { onAccount: delegateKeypair });
      await contract.methods.add_key(nameOwnerKeypair.publicKey, testName, channelPointerKey, {'AENS.ChannelPt':[nameOwnerKeypair.publicKey]}, aensDelegationSignature, { onAccount: delegateKeypair });
      let aensName = await contract.methods.get_name(testName);
      console.log(aensName.decodedResult);
      pointers = await contract.methods.get_pointers(testName);
      console.log(pointers.decodedResult);
  });

test output

Map(0) {}
{
  'AENS.Name': [
    'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3',
    { FixedTTL: [Array] },
    Map(4) {
      'channel' => [Object],
      'oracle_pubkey' => [Object],
      'account_pubkey' => [Object],
      'contract_pubkey' => [Object]
    }
  ]
}
Map(4) {
  'channel' => {
    'AENS.ChannelPt': [ 'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' ]
  },
  'oracle_pubkey' => {
    'AENS.OraclePt': [ 'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' ]
  },
  'account_pubkey' => {
    'AENS.AccountPt': [ 'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' ]
  },
  'contract_pubkey' => {
    'AENS.ContractPt': [ 'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' ]
  }
}
      ✔ should add and get pointers correctly (801ms)

I think it's really inconvenient to provide a variant for oracle, contract or channel which has the ak_ prefix in the required value. probably FATE internally represents it like this.

ideally the map of pointers would look like this:

Map(4) {
  'channel' => 'ch_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3',
  'oracle_pubkey' => 'ok_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3',
  'account_pubkey' => 'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3',
  'contract_pubkey' => 'ct_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3'
}

what do others think regarding this? make we can introduce some helper functions for transforming functions in both directions.

we should also consider https://github.com/aeternity/aepp-sdk-js/issues/1240 when addressing this. maybe even independent of this. currently we still don't allow custom keys in the SDK.

cc @dincho @mradkov @davidyuk

marc0olo avatar Nov 24 '21 00:11 marc0olo

I agree with @marc0olo here, we should definitely adjust that from wherever that is originating. All the http endpoints of the node also expose the correct id addresses for each type. This has to be consistent across the ecosystem, otherwise it will confuse eth experienced devs even more.

thepiwo avatar Nov 24 '21 08:11 thepiwo

Looks like an issue on Sophia side, AENS pointers are actually defined this way, probably there was a reason to do so.

davidyuk avatar Nov 24 '21 08:11 davidyuk

I guess we have to find a solution in the SDK 🤔

marc0olo avatar Nov 24 '21 09:11 marc0olo

I confirm it's a variant as you already found :)

What about changing your contract actually and return the structure you'd like ?

dincho avatar Nov 24 '21 09:11 dincho

Also as we already destroyed the calldata lib consistency we might think of minimizing the variant structure in edge cases, i.e. variant constructors with single argument can be represented by {Ctor: value} ?

That won't get you to the ideal structure you're expecting tho, but:

  'channel' => {
    'AENS.ChannelPt':  'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' 
  },
  'oracle_pubkey' => {
    'AENS.OraclePt':  'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' 
  },
  'account_pubkey' => {
    'AENS.AccountPt':  'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' 
  },
  'contract_pubkey' => {
    'AENS.ContractPt':  'ak_2VARB6P7nzKt4VvaPYLYVQgRfE92ahJJoYWuNc6eUvFSYgYiR3' 
  }

Which still does not make it "nice", indeed it needs SDK work or contract changes IMO

dincho avatar Nov 24 '21 09:11 dincho

@hanssv what are your thoughts about this? we cannot do anything about this representation on Sophia side, right? what's the reason to have it like this? I guess there is one :-)

marc0olo avatar Nov 24 '21 10:11 marc0olo

What about changing your contract actually and return the structure you'd like ?

that's an interesting point. what do others think about this? maybe we should provide helper functions in the standard library to do the conversion. might be the easiest solution for us. though it sounds a bit dirty to me, still.

marc0olo avatar Nov 24 '21 10:11 marc0olo

Do you all agree also that IT-technology is a good thing to write? 😉

The main confusion point here, I think, is that ak_... has come to mean both account and address of on-chain entity. Here you need to (mentally) separate the address of the oracle/contract(/channel) from the oracle/contract. ok_... is an oracle, ct_... is a contract; ak_... is an address to something - an oracle, a contract or just an account (or all three!!).

In Sophia everything is strongly typed, an oracle has a particular request/response type, and a contract is typed by its interface - therefore we cannot have a generic oracle type, or a generic contract type. (And hence we can't have a AENS.Pt that takes either an account, an oracle, a contract, or a channel).

So I would strongly advise against this "helping" behavior; I think, at the end of the day it will cause even more confusion.

hanssv avatar Nov 24 '21 10:11 hanssv

I think it's really inconvenient to provide a variant for oracle, contract or channel which has the ak_ prefix in the required value. probably FATE internally represents it like this.

Oh, I've missed that part, but Hans already explained

dincho avatar Nov 24 '21 12:11 dincho

I think it's really inconvenient to provide a variant for oracle, contract or channel which has the ak_ prefix in the required value. probably FATE internally represents it like this.

Oh, I've missed that part, but Hans already explained

I think we know about the main confusion point that Hans mentioned here. at least I hope that everybody is aware of that. but it's generally a topic that can be confusing in many ways. I am really not sure about the best way to deal with it. maybe we should really leave it like is. but as you can see in the initial post of the issue this isn't really convenient.

the main question for me here is:

  • what's the purpose of a pointer of different first class objects?

I would argue that every aepp developer expects to set/receive ok_ (for oracle), ch_ (for channel) and ct_ (for contracts) if he actually uses pointers for different first class objects.

in the current state I would have to take the pointer type and convert the address of on-chain entity to have the respective prefix I need. maybe this is fine. but then the SDK should provide a helper function to do that. same for transforming to the correct param type expected by calldata lib

marc0olo avatar Nov 24 '21 12:11 marc0olo

It's like interface and implementation isn't it ? (abstract/concrete type)

given the signiture: call(Address: a): Address a

and client code:

Oracle o
Address a
a = obj.call(o)

it should be in the client domain to know the concrete type of a and can "cast" it:

o = (Oracle) a

This is how I understand it.

That said I agree that clients should have a way to convert/cast it

P.S. no, the example code is not in any lang :)

dincho avatar Nov 24 '21 13:11 dincho

Yes, it comes down to concrete type versus the (absence) of abstract type.

Let's assume we have a contract type: contract interface C = ... Now if we want to store this in a pointer in a contract, we have two options:

entrypoint alternative_1(c : address) : AENS.pointee = AENS.ContractPt(c)

entrypoint alternative_2(c : C) : AENS.pointee = AENS.ContractPt(c.address)

Where alternative_1 should be called with ak_... and alternative_2 with ct_.... What should be noted here is that the pointer carry no type information, and the only way to get back the a value of type C is to explicitly cast it:

entrypoint foo(c : C) : C = 
  let pt = alternative_2(c)
  let AENS.ContractPt(a) = pt
  Address.to_contract(a)

hanssv avatar Nov 24 '21 13:11 hanssv

I think we shouldn't hide the concrete type / variant and just provide helper functions to convert/cast. we will always face problems if we try to hide facts.

if we provide good examples / docs around that we should be fine.

can we agree on that? 😅

note

  • we should consider that we didn't even support this so far at all (!)

marc0olo avatar Nov 24 '21 14:11 marc0olo