search

WalletConnect / kotlin-walletconnect-lib

library to use WalletConnect with Kotlin or Java
MIT License
156 stars 98 forks source link

session callback change #17

Closed ligi closed 5 years ago

ligi commented 5 years ago

follow up to #13 that introduced transportStatus and made the inconsistent callback even more inconsistent:


    interface Callback {
        fun transportStatus(status: Transport.Status)

        fun handleMethodCall(call: MethodCall)

        fun sessionApproved()

        fun sessionClosed()
    }

I suggest either add handleXX everywhere or remove it for handleMethodCall might go even further here (and reason this is an issue and not a PR) we could reduce to 2 methods: handleStaus and handleMethodCall and the session open closed events then come via the status

rmeissner commented 5 years ago

Yes this callback interface is not really nice, I am fine either way (prefer less method a little bit more).

Android often also uses onXX ... not sure if we would want to go for that 🤔 ... but as said no strong opinion (should just be consistent as you said 😄 )

ligi commented 5 years ago

great - will make a pr with onStatus and onMethodCal then. Would you be on some chat for another quick question?

rmeissner commented 5 years ago

sure :) just send an email with a communication channel (if not here ;) )

rmeissner commented 5 years ago

Hey, we would love to release the fix next week, therefore I would work on this tomorrow if you don't have time :)

proposed interface:

interface Callback {
   fun onStatus(status: Session.Status)

   fun onMethodCall(call: MethodCall)
}

sealed class Status {
   object Connected: Status() // Transport is connected
   object Disconnected: Status() // Transport is disconnected
   object Approved: Status() // Session was approved
   object Closed: Status() // Session was closed
   data class Error(val error: Throwable): Status() // Propagate error
}

Edit: alternative wording for Status would be Update from my side (not sure if that is really better :/ )

ligi commented 5 years ago

Interface looks good to me - and happy you will work on it as I could only do it next week - this week was(is) full with stuff.

rmeissner commented 5 years ago

Should we tag a version with this? (0.10.0)

ligi commented 5 years ago

Yes we can do - but would love 15 more minutes as I am testing around with it currently a bit more. If you need it earlier - feel free to tag

rmeissner commented 5 years ago

Only need it on monday ;) so no time pressure, was just checking if we want more obvious changes included :)

tschubotz commented 5 years ago

@ligi When do you think will you be able to tag a version that contains this fix? :)

We are planning to do a release of the Gnosis Safe by Thursday this week and would love to have it fix the currently broken WalletConnect integration :)

(Didn't want to bother Richard with this as he's on vacation currently.)

cc @elgatovital

ligi commented 5 years ago

oh sorry - tagging a version now - got distracted over the weekend (https://twitter.com/mr_ligi/status/1153606842855149568)

ligi commented 5 years ago

released 0.9.3 but there is still something off - but it seems to be unrelated to the kotlin-lib - also used the old version against the example and things failed. So I think it is something on the WalletConnect bridge server or the example. Wanted to investigate more before setting the tag on Friday - but got distracted. But still tagged it so it is easily accessible for your release. It is also not making things worse as far as I saw. Hope I can give it some more debugging today to really know where the problem lies.

tschubotz commented 5 years ago

hah, no worries, nice hackathon project!

Thanks for the release!

pedrouid commented 5 years ago

Hey @ligi, what is the weird behavior that you're seeing with kotlin lib? There were changes with the Bridge server last week. Most notably, the ping opcode which wasn't present before and the socket messages now include a boolean parameter called silent. Do any of these seem to be the cause of it?