Remove the controller from storage #174
Conversation
ramtinms
requested review from
sideninja,
psiemens,
turbolent and
janezpodhostnik
as code owners
There was a problem hiding this comment.
This looks good.
The concern i have is that anyone using persistent storage will have to start fresh, as the reads are no longer compatible.
One solution would be to fetch registers at both the old key and the new key, but we would have to keep that code in for a long time.
The other solution would be to offer some persistent storage migration tool or maybe a guide guide or just automatically migrate persistent storage at first load.
ramtinms
changed the title
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
==========================================
- Coverage 51.90% 51.76% -0.14%
==========================================
Files 29 29
Lines 2335 2345 +10
==========================================
+ Hits 1212 1214 +2
- Misses 984 991 +7
- Partials 139 140 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| c := strings.Count(registerString, "-") | ||
| if c == 3 { // old format | ||
| parts := strings.SplitN(registerString, "-", 3) | ||
| if len(parts) < 3 { | ||
| return flowgo.RegisterID{}, fmt.Errorf("failed to parse register ID from %s", string(key)) | ||
| } | ||
| return flowgo.RegisterID{ | ||
| Owner: parts[0], | ||
| Key: parts[2], // skip the controller | ||
| }, nil | ||
| } | ||
| // else is the new format |
There was a problem hiding this comment.
Do we need this backwards-compatibility. Can we just remove it?
There was a problem hiding this comment.
I thought we don't need it initially, but Janez is right people might have use persistant storage that have these values, it seems there is no harm to keep it in case we have to load things.
|
@ramtinms It looks like this branch references flow-go commit 7015f054a69f that isn't on After you merge that PR, can you update the emulator to import from In the future, please avoid merging an emulator PR the depends on unmerged code in flow-go 🙏 |
|
@psiemens good point, going to update this as soon as the other PR is merged, I also going to create an issue so we could remove the dependency of flow-go to flow-emulator, that seems not natural. |
|
updated here #177 |
Description
This PR
For contributor use:
masterbranchFiles changedin the GitHub PR explorer