search

cosmos / iavl

Merkleized IAVL+ Tree implementation in Go
Apache License 2.0
422 stars 263 forks source link

feat: ADR for api cleanup and async commit #790

Closed cool-develope closed 1 year ago

ValarDragon commented 1 year ago

I'd generally still suggest starting with separating out version management (#737 )

cool-develope commented 1 year ago

I'd generally still suggest starting with separating out version management (#737 )

@tac0turtle @yihuang , how about separating out version manager? After Saveversion with batch, we will have only version-related functions as a modifier. IMO, separated interfaces looks meaningless in the store module.

yihuang commented 1 year ago

I'd generally still suggest starting with separating out version management (#737 )

@tac0turtle @yihuang , how about separating out version manager?

After Saveversion with batch, we will have only version-related functions as a modifier. IMO, separated interfaces looks meaningless in the store module.

I'd suggest figure out the minimal changes needed to support store v2 first (async commit, atomic commit), I don't see a big problem in current API design. The MutableTree is a singleton, so I think it's not too bad for it to also do the version manager job, I'm ok either way :)

tac0turtle commented 1 year ago

lets separate concerns, first what is needed for store v2 then a new api. I think conflating the two could be clouding what is all needed

cool-develope commented 1 year ago

Do we really require the Immutable Tree in Store V2, considering that iavl will solely be utilized for SC? cc: @alexanderbez @tac0turtle

tac0turtle commented 1 year ago

im still a bit lost with this adr, API cleanup has nothing to do with store v2. The commitment structure should be agnostic to what is going on at a higher level. I think we should remove any notion of store v2 and the plans there and work on creating a stable and sane API for this code base

cool-develope commented 1 year ago

im still a bit lost with this adr, API cleanup has nothing to do with store v2. The commitment structure should be agnostic to what is going on at a higher level. I think we should remove any notion of store v2 and the plans there and work on creating a stable and sane API for this code base

ok, I modified the ADR, and assumed ImmutableTree is still necessary.

cool-develope commented 1 year ago

just removed the parallel WorkingHash part, I think it is unnecessary since we can parallelize on a module basis. Regarding the naming convention (Reader/Writer), we can organize it on the consumer side, I'd like to keep the current structure.

cool-develope commented 1 year ago

it is not iavl 2.0, just simple API cleanup @kocubinski is currently working on iavl 2.0