fftimestamp fix, SQL DB persistence implementation

[Philip-21]

This is pr fixes for #111

[Philip-21]

Hey @Chengxuan does this adjustment tackle the issue ?

[hanqidd123]

I also took a look at this issue. I think there is another implementation of the interface in leveldbPersistence. Does that function need to be changed as well?

[Chengxuan]

@Philip-21 the proposed change looks good to me. As @hanqidd123 pointed out, the public interface itself will need to be changed, as well as other implementations and the consumer side of the code.

I've approved the GitHub run so that you can see the areas that also need to be changed, the consumer code of the method is in the simple transaction handler which is also in this repo. So you should be able to make all the changes in the same PR.

[Philip-21]

Hey @Chengxuan I have been able to fix the DB persistence issue, I created a separate SQLPersistence interface for handling all DB transactions accross the codebase
Using the Persistence interface won't work because i would have had to define all the methods located in the Persistence interface for the type sqlPersistence struct{} to initialize, so i decided to create a separate SQLPersistence interface and define all the methods so the type sqlPersistence struct{} can intialize properly

[Chengxuan]

@Philip-21 have you tried updating the existing common interface instead of creating a new one?

[Philip-21]

@Philip-21 have you tried updating the existing common interface instead of creating a new one?

Yes @Chengxuan I did but, it still boils down to defining all the methods located in the Persistence interface for the
type sqlPersistence struct{}

The methods for each interface in the Persistence interface have their own receivers for implementing each of them.

If i define the Sqlpersistence interface into the Persistence interface i would have to implement all the other methods not relating to the Sqlpersistence interface, these methods will have type sqlPersistence struct{} initialized as its receiver, When implementing the NewPostgresPersistence()

[Chengxuan]

https://github.com/hyperledger/firefly-transaction-manager/blob/main/pkg/txhandler/txhandler.go is where the common interface gets defined

[Philip-21]

https://github.com/hyperledger/firefly-transaction-manager/blob/main/pkg/txhandler/txhandler.go is where the common interface gets defined

Hey @Chengxuan i tried setting up the SQLinterface in the common interface but i kept getting import cycle errors as i tried to import some packages to be used in defining the methods

[Chengxuan]

Hi @Philip-21 I suggest you start fresh from the main branch, and update the signature of AddSubStatusAction in https://github.com/hyperledger/firefly-transaction-manager/blob/main/pkg/txhandler/txhandler.go#L73 first. Then the golang linter should point you to the other places that require to be changed. You don't need to declare any new interface.

[Philip-21]

Hi @Philip-21 I suggest you start fresh from the main branch, and update the signature of AddSubStatusAction in https://github.com/hyperledger/firefly-transaction-manager/blob/main/pkg/txhandler/txhandler.go#L73 first. Then the golang linter should point you to the other places that require to be changed. You don't need to declare any new interface.

Will do that, apologies for the late reply

[Philip-21]

Hey @Chengxuan i have worked on the AddSubStatus() signatures and also made corrections inicated by the golang-ci lint

[Chengxuan]

Hi @Philip-21 , your linter configuration seems doesn't match the version specified in the Makefile. You shouldn't need to update all the license headers. Please fix it.

Also, the latest change doesn't look correct, the interface still doesn't support time to be passed in.

Please let me know if you'd like me to raise a PR to demonstrate how to tackle this. I don't want you to struggle too much with it.

[Chengxuan]

@Philip-21 sorry don't see your comment here but I did receive a notification:

#117

Closing this PR in favor of the one above

[Philip-21]

@Philip-21 sorry don't see your comment here but I did receive a notification:

#117

Closing this PR in favor of the one above

Please can i give it another try in another pr, before asking for help ?

[Chengxuan]

Hi, @Philip-21 , sorry, I thought it was a Github issue as it's been slow recently. I've raised the PR so feel free to read and approve.

Thanks for all the effort made in the PR.

I also understand the disappointment of a long PR like this being closed, I hope you are proud of all the learnings on this PR as I'm sure they'll be valuable for your future contribution. 👍

[Philip-21]

Yes i have learned, thank you for putting me through @Chengxuan