escrow bucket.Create does not save

[husio]

In package x/escrow, Bucket.Create is described to be saving the created instance (see comment), but it only creates an ID and returns entity without storing it.

Method is used in creation handler and it seems to expect the instance to be stored in the database when Create is called.

[ruseinov]

@husio It happens in Deposit, so all good. Just looks a bit confusing.

[husio]

Good point.

I think the comment of the Create method should be changed then, because right now it is

// Saves the object and returns it (to inspect the ID)

[ruseinov]

@husio sure, I'm pretty sure this was copy-pasted from other implementations as this is a pretty standard workflow.

[ruseinov]

There's more where this is coming from, might be helpful to go through the code and see where else we are missing/have invalid comments. Makes extension development easier for newcomers and just in general.

[ethanfrey]

Yeah, this is a bit confusing. Just update to write to the db in Create as well.

And a cleanup of comments is always welcome. Most of this code was writen quite quickly when I was the only one reading/writing the project, so happy for any fixups.

I think we are not too worried about optimizing writes yet, and they go to a memory db now. You can update this to Save in create as well (and maybe save a second time). There is an old issue to optimize the CacheWrap, so for any given key we only write at most once per block to the underlying leveldb on Commit. Right now we will replay all writes for all successful transactions, but could safely be squashed to the last write for a given key, when we need to squeeze some more performance.

[alpe]

As @husio brought this up, now. Some definition for create in this project would make sense for me. I saw this with save and without. Especially in context of nft we create first and save in a second step as some data is set/modified on the object. This can be changed by passing all data though.

[ethanfrey]

I'm fine with any usage, but clarity and consistency is better.

It seems like we would assume a save in create, but we could also document otherwise. I think I originally conceived it as not saving, but I probably mixed it. I will defer to your decisions, but consistency is good, which this issue properly points out.

[husio]

I think it is less surprising if Create is saving an entity. It is common for ORMs to work this way. If it is not intended to save it, maybe the method could be renamed to Build or New?