Blue account #1033
Blue account #1033
Conversation
| Requests []Request | ||
| AfterTest func(t *testing.T, db weave.KVStore) | ||
| }{ | ||
| "configuration can be updated": { |
There was a problem hiding this comment.
Please add success to the beginning of the test case name so reading the test results will become easier.
There was a problem hiding this comment.
I agree that adding "success" might help with readability. I do not know where to add it so that it makes sense.
I think you meant prefixing all "successful" tests with "success:". Initially I thought this is a great idea. Now that I am adding it, I am not so sure anymore. The reason is that scenario itself is neither success or failure (unless we talk about the test result). A test scenario in this case consuming messages in order and a certain state at the end is expected. In the process of consuming messages, success of failure is expected as well. I don't think this can be generalized to the whole test description.
In the case of configuration can be updated, adding success: prefix makes it look weird:
success: configuration can be updated
Now it looks like I expect the test to succeed. This is true, because I expect ALL tests to succeed 🙈 In my opinion adding success: does not provide any information and can only confuse.
What do you think?
There was a problem hiding this comment.
IMO success or failure outcome must solely depend on the expected test outcome as you expect this test to successfully validate the values returned from config. Basically happy path
For the format:
I think it is better to only insert the prefix success or failure because when a test explodes the console output will be success:_configuration_can_be_updated. : looks weird. And for the sentence structure, I prefer success update configuration. Dead simple, no past or future tenses.
There was a problem hiding this comment.
Ok, now I get it. Makes sense and we can go this direction.
The only complain I have is that success update configuration does not sound like a correct phrase. How should I rephrase those descriptions so that they do not sound weird 🇬🇧 ?
There was a problem hiding this comment.
Does it have to be correct and sound English?
There was a problem hiding this comment.
I think yes. It might be very confusing otherwise. I believe it should briefly describe the test case.
There was a problem hiding this comment.
@willclarktech please save us from this misery 😢
There was a problem hiding this comment.
I'm not convinced you need these markers at all because it's clear to me at least that "configuration can be updated" is a happy path. If you want to improve visual grepping to distinguish happy paths from unhappy paths, why not prefix with "happy path:"/"unhappy path:" or better yet with 😄 and 😞?
There was a problem hiding this comment.
I will continue tomorrow. The first round of review:
- You made some typos around the code such as
transferDomaiHandler. Also in some comments. Maybe we can add a go comment spelling checker to builds. - Model and message tests are missing. If you are running out of time, It is okay to leave a few technical debts.
- Comment about
validationlogic is a very important one. This one absolutely should be fixed.
| if _, err := h.validate(ctx, db, tx); err != nil { | ||
| return nil, err | ||
| } | ||
| return &weave.CheckResult{GasAllocated: 0}, nil |
There was a problem hiding this comment.
I think domain registration must cost a fee. https://github.com/iov-one/weave/blob/master/cmd/bnsd/x/username/handlers.go#L36
There was a problem hiding this comment.
I don't understand what those fees mean anymore. We have x/msgfee to handle both anti spam and product defined fees. Do you know why do we need additional hardcoded fee?
Ethan explain me this today. In german, so maybe I got some parts wrong 😅
The bottom line is that we do not support Gas, full integration was never finished which renders it useless to us. We set Gas as arbitrary values, without any bigger consideration of what those values means.
Gas is used by Tendermint. It is part of the ABCI and should be used to communicate the cost of processing a transaction. Cost as the computational effort, for example CPU usage or database writes.
Tendermint provides additional functionality that utilize Gas information - you can setup maximum Gas per block. As far as I know we do not use it.
There was a problem hiding this comment.
The outcome of the discussion is very productive and this should be documented as well.
Database cannot be accessed while an iterator is in use.
This comment has been minimized.
This comment has been minimized.
update `cmd/bnscli` to support more transactions temporary solution to initalizing blueaccount extension configuration.
| pkg string, | ||
| config OwnedConfig, | ||
| auth x.Authenticator, | ||
| initConfAdmin func(weave.ReadOnlyKVStore) (weave.Address, error), |
There was a problem hiding this comment.
@ethanfrey @alpe Please have a look if you have a moment. I think this is a rather big change to weave configuration patterns.
This is solving the problem I have described in the comment above - I want to create a configuration and cannot authenticate, because configuration entity defines who can authenticate 🐣
Usually configuration is created via genesis. This is not possible if you add an extension to an existing state.
It is not possible to do this via migrations as Ethan suggested for two reasons:
- migrations do not have access to the context. This is on purpose as the code that runs them might not have access to the context as well (i.e. bucket),
- migrations are given only
weave.ReadOnlyKVStore. This is also on purpose, so that they are not able to modify the database state (!). They are supposed to migrate the in memory entity representation.
I require explicit passing of an admin address function instead of directly importing migration.loadConf in gconf because of import cycle. I think it is also nice that initConfAdmin is optional.
What do you guys think?
|
This pull request is becoming a general fix change. I have noticed a few missing commands in |
Blue Account functionality implemented as described in #1031
Implementation follows closely acceptance criteria.