feat: Update datastore packages to allow use of context

[AndrewSisley]

Closes #44

Most of the changes are just passing around a context object, however there are some more risky changes dotted around due to changes made to some of the dependencies:

• dshelp.DsKeyToCid(key) was replaced with a function that converted to a V1 Cid, there is no helper for the V0 CIDs that we use in the headstore, this is probably the riskiest change, as our key versioning seems to be a bit messy (constructing v1 prefixes and then using then to construct v0 keys - e.g. collection.go ln 297-309), and there are fewer unit tests covering the commit queries - I changed the heads_test.go tests to reflect the use of v0 keys - this should be the only test change besides context handling
• Similar to the above dshelp.CidToDsKey(cid) was replaced by dshelp.MultihashToDsKey(cid.Hash())
• I deleted a public function as I got too lazy to maintain it, let me know if we want to keep it

[jsimnz]

Thanks! I'll get on this ASAP as I def want to get the data store upgrade in too

…

On Wed, Nov 17, 2021, 10:44 PM AndrewSisley ***@***.***> wrote: @AndrewSisley <https://github.com/AndrewSisley> requested your review on: #48 <#48> feat: Update datastore packages to allow use of context. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6UKNVGEJLBPOBJSH7RGZTUMRZCTANCNFSM5IIS4XLQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jsimnz]

Wow that's quite an overhaul.

I really hate to do this to you, but the standard practise for using the context package is to use the code:

ctx := context.Background()

So, the instantiated context variable is ctx. The current way your doing it means we are overwriting the symbol for the context package with the specific instance of the context variable.

Sorry :)

[jsimnz]

I think you might have got lost in translation with respect to the Cid and DocKey versioning. I like that you updated dshelp package since I was using a pretty outdated version.

We use the CidV1 exclusively, but we have a defined version system for the DocKey as well, which is the DocKeyV0. The versions are independant of one another.

So with that being said, the upgrade to the newer dshelp package, you use the NewCidV0 function but we should instead be using

bk, err := dshelp.BinaryFromDsKey(key)
k := cid.NewCidV1(cid.Raw, bk)
...

[AndrewSisley]

Wow that's quite an overhaul.

I really hate to do this to you, but the standard practise for using the context package is to use the code:

ctx := context.Background()

So, the instantiated context variable is ctx. The current way your doing it means we are overwriting the symbol for the context package with the specific instance of the context variable.

Sorry :)

Ah no worries - I wondered if you'd flag that - I do prefer the long name, but will make the change :)

[AndrewSisley]

I think you might have got lost in translation with respect to the Cid and DocKey versioning. I like that you updated dshelp package since I was using a pretty outdated version.

We use the CidV1 exclusively, but we have a defined version system for the DocKey as well, which is the DocKeyV0. The versions are independant of one another.

So with that being said, the upgrade to the newer dshelp package, you use the NewCidV0 function but we should instead be using

bk, err := dshelp.BinaryFromDsKey(key)
k := cid.NewCidV1(cid.Raw, bk)
...

The newer dshelp was required when using the newer datastore packages - code didn't compile without it (there were a couple I had to update manually). I think I understand what you mean - will give it a go and then read up Dockey versions :)

[AndrewSisley]

Flipped it back to V1, and the commit query tests started failing again - maybe we can have a quick call about this?

V1 cids seem to look like this:
bafkreifvdqhkkvurzwphw27gwhyazybqajqtwrntfacpwfziy3tdo7v5hi

But the Headstore is used to:
QmaXdKKsc5GRWXtMytZj4PEf5hFgFxjZaKToQpDY8cAocV

Yesterday I ended up comparing the bytes and different serializations - it does look like the 'new' V1 code is adding a new identifier at the start, which I assumed was the new version id (not present in v0?)

EDIT: I've pushed a new commit showing the problem (tests fail)

[jsimnz]

Flipped it back to V1, and the commit query tests started failing again - maybe we can have a quick call about this?

V1 cids seem to look like this: bafkreifvdqhkkvurzwphw27gwhyazybqajqtwrntfacpwfziy3tdo7v5hi

But the Headstore is used to: QmaXdKKsc5GRWXtMytZj4PEf5hFgFxjZaKToQpDY8cAocV

Yesterday I ended up comparing the bytes and different serializations - it does look like the 'new' V1 code is adding a new identifier at the start, which I assumed was the new version id (not present in v0?)

EDIT: I've pushed a new commit showing the problem (tests fail)

You are correct regarding the CID version issue. Lets revert the last commit to as you had it. The older dshelp package got me mixed up with respect to the version structure.

Yes, for now lets keep things using CidV0.

As for the context vs ctx, its just very standard Go practise, and beyond that we don't want to overwrite the context package.

IE:

import "context" // context refers to package 
func main() {
    context := context.Background() // create a context from the package, called context
    
    // this next line will fail to build, since "context.WithTimeout" is trying to 
    // refer to the global WithTimeout function defined in the context 
    // package, but context refers to the variable we created above.
    context = context.WithTimeout(context, time.Second) 
    someFunc(context)
}

vs

import "context" // context refers to package 
func main() {
    ctx := context.Background() // create a context from the package, called context
    
    // now it builds and is clear whats happening
    ctx = context.WithTimeout(ctx, time.Second) 
    someFunc(context)
}

[AndrewSisley]

Variables renamed to ctx - think I was being unfairly grumpy+stubborn with Go, I don't think C#/Rust would let you compile at all with a name clash (although collisions are rarer due to casing/import styles...) :) I need to keep an eye out for that unfairness perhaps.

Temp commit that explained the cid/ds.Key issue dropped.

Ready for re-review/merge when you are.