-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(transcation):#195 fixed min_confirmation bug on verify transaction #199
Conversation
…error message in Test_Sharder_Verify_Txn_Failed
…error message in Test_Sharder_Verify_Txn_Failed
Need to make corresponding changes to For testing use go get github.com/0chain/gosdk@5cd687300fd776d80f3dada97bfdc210955380a0 in |
Dayi Chen 23 hours ago Piers Shepperson 22 hours ago Dayi Chen 22 hours ago Dayi Chen 22 hours ago Piers Shepperson 22 hours ago Dayi Chen 22 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Dayi Chen 21 hours ago Piers Shepperson 21 hours ago Dayi Chen 20 hours ago Dayi Chen 20 hours ago Dayi Chen 20 hours ago Dayi Chen 20 hours ago Dayi Chen 20 hours ago Dayi Chen 20 hours ago Dayi Chen 3 hours ago Piers Shepperson 2 hours ago Dayi Chen 2 hours ago Piers Shepperson 35 minutes ago Piers Shepperson 32 minutes ago Dayi Chen 21 minutes ago Dayi Chen 21 minutes ago Dayi Chen 20 minutes ago Piers Shepperson
Dayi Chen 20 minutes ago Dayi Chen 16 minutes ago Piers Shepperson 15 minutes ago Dayi Chen 14 minutes ago Piers Shepperson
Dayi Chen 14 minutes ago Dayi Chen 13 minutes ago Dayi Chen 10 minutes ago Dayi Chen 8 minutes ago Piers Shepperson 8 minutes ago Dayi Chen 7 minutes ago |
I would suggest to upgrade gosdk first. And upgrade zbox/zwallet after then. Nothing will be broken. Because zbox/zwallet is working on current gosdk version before we upgrade it. |
if we have zbox/zwallet upgrade with local version, could you let me know we should how to review the PR on zbox/zwallet without new gosdk? |
I don't like the idea of unnecessarily merging untested code. |
Is it a circle? |
#199 (comment) |
all unit tests are passed ,right? https://github.com/0chain/gosdk/actions/runs/1119656429 |
All unit tests have passed, but this is mainly a regression test. Integration testing is necessary to test new gosdk changes work. All PRs should be integration tested on a running chain before merging. This goes double for |
We have raised an upgrade task on zbox 0chain/zboxcli#82 . Is it still not ok? They can be done one by one , right? |
Not sure what you mean.
|
Why we need version management on gosdk? Is it easy and standard flow
1) at first, upgrade gosdk with bug fixes and new feature
2) review and merge changes on gosdk, and release a new version
3) and then , we upgrade zbox with new gosdk
Why we need to do 1,3 and 2? I can’t understand it. Is it(1,3 and 3) better?
From mobile
…------------------ Original ------------------
From: Piers Shepperson ***@***.***>
Date: Wed, Aug 11, 2021 9:28 PM
To: 0chain/gosdk ***@***.***>
Cc: Lz ***@***.***>, Author ***@***.***>
Subject: Re: [0chain/gosdk] fix(transcation):#195 fixed min_confirmation bug on verify transaction (#199)
Not sure what you mean.
These changes need to be integration tested.
We need to make changes to zbox and zwallet to implement thisPRs changes.
Modify the go.mod and go.sum files in zbox and zwallet to point to this PR's gosdk branch for testing #199 (comment).
Integration the new gosdk, zbox and zwallet changes.
Once integration testing is successful the gosdk changes can be merged into staging.
The zbox and zwallet PRs will need to be changed so the go.mod and go.sum point to the new gosdk head . Use go get github.com/0chain/gosdk@ again.
zbox and zwallet can now be merged.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
The gosdk code has not been tested yet. I don't see any good reason to unnecessarily merge untested code. Also, I have been asked not to merge any code that I have not personally tested on a running chain, and I can't do this unless there is a compatible zbox or zwallet. |
The process is probably easier than it seems. Once you get used to using go get github.com/0chain/gosdk@<commit> it's quite straightforward. |
Not, I get it worked with go mod replace on local. But it doesn’t work with our current GitHub workflow. That is why I can’t do it like you want.
I don’t know how to continue this issue. If possible, could you get it finished as an example? I will follow you next issue, thanks.
From mobile
…------------------ Original ------------------
From: Piers Shepperson ***@***.***>
Date: Wed, Aug 11, 2021 9:48 PM
To: 0chain/gosdk ***@***.***>
Cc: Lz ***@***.***>, Author ***@***.***>
Subject: Re: [0chain/gosdk] fix(transcation):#195 fixed min_confirmation bug on verify transaction (#199)
The process is probably easier than it seems. Once you get used to using
go get github.com/0chain/gosdk@<commit>
it's quite straightforward.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks fine, just minor code cleanup
Code should be reiviewed after it has been tested and shown to work. You have a soultion that might work the next step is to test it. untill it is tested there is no expectation that the code works. So i suggest making the necessary changes to zbox and zwallet, creating Prs with thoes changes, starting up a chain and using zbox to post some transactions. You can then singel step though your code and check that it works.That will make fixing any issues that much easier. |
Hi man, it is done . And you can check it from git if you don’t believe it.
|
Great, you have created zbox and zwallet PRs. I should be testing some issues later, I'll take the opportunity to check the PRs' code then. |
I tested zwallet 0chain/zwalletcli#36 and the changes work fine. If I understand correctly there are a subset of Do we have this documented anywhere, with a list of API methods that require a previous call to |
Don’t worry. An ErrConfigIsNotInitialzed will be thrown. Do you review those code? Or just do test job?
From mobile
…------------------ Original ------------------
From: Piers Shepperson ***@***.***>
Date: Sun, Aug 15, 2021 5:10 PM
To: 0chain/gosdk ***@***.***>
Cc: Lz ***@***.***>, Mention ***@***.***>
Subject: Re: [0chain/gosdk] fix(transcation):#195 fixed min_confirmation bug on verify transaction (#199)
I tested zwallet 0chain/zwalletcli#36 and the changes work fine.
If I understand correctly there are a subset of gosdk API methods that require a previous call to conf.LoadConfigFile so gosdk has access to the config details.
Do we have this documented anywhere, with a list of API methods that need the config details pre-loaded?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
It is good that the correct error message is thrown. I assume you have integration tested the other parts of the PR. I just checked that the changes were not breaking general usage for other developers. Code reviews aren't used to check the code works, testing does that. However, this PR introduces changes to how As this is a public API it would be nice to add these details to the documentation to be picked up by But check with @saswata to see if there is anywhere else that documented gosdk and needs to be updated. |
fixed #195 ,and improved error message