cspr-rad / kairos

Apache License 2.0
2 stars 0 forks source link

Deposit contract suggestions #95

Closed marijanp closed 4 months ago

marijanp commented 4 months ago

Incorporate refactorings after discussions with @Avi-D-coder.

Merge this branch into feature/deposit-contract then merge feature/deposit-contract into master.

Or directly merge this.

github-actions[bot] commented 4 months ago

File Coverage
All files 57% :x:
kairos-crypto/src/implementations/casper.rs 6% :x:
kairos-test-utils/src/cctl/parsers.rs 66% :white_check_mark:
kairos-tx/src/asn.rs 51% :x:
kairos-tx/src/error.rs 0% :x:
kairos-server/src/routes/deposit.rs 88% :white_check_mark:
kairos-server/src/routes/transfer.rs 90% :white_check_mark:
kairos-test-utils/src/cctl.rs 90% :white_check_mark:
kairos-server/src/state/transactions.rs 66% :white_check_mark:
kairos-server/src/state/trie.rs 35% :x:
demo-contract-tests/tests/test_fixture/mod.rs 97% :white_check_mark:
kairos-server/tests/transactions.rs 85% :white_check_mark:
kairos-server/src/state/transactions/batch_state.rs 36% :x:
kairos-server/src/config.rs 0% :x:
kairos-server/src/errors.rs 12% :x:
kairos-server/src/lib.rs 95% :white_check_mark:
kairos-server/src/state.rs 92% :white_check_mark:
kairos-server/src/utils.rs 22% :x:

Minimum allowed coverage is 60%

Generated by :monkey: cobertura-action against b663b89cb45a04c4a27633ea7859b5a646f7c51a

jonas089 commented 4 months ago

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

marijanp commented 4 months ago

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

The idea is to get the requested changes incorporated and finallize the deposit contract. After we reach a state that addresses all the problems, the idea is to merge this into the feature/deposit-contract branch.

It would be great if you could answer some of the review comments here.

koxu1996 commented 4 months ago

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

The idea is to get the requested changes incorporated and finallize the deposit contract. After we reach a state that addresses all the problems, the idea is to merge this into the feature/deposit-contract branch.

It would be great if you could answer some of the review comments here.

Sounds good, however I am confused by current target branch:

image

@marijanp Could you update base into feature/deposit-contract?

Right now reviewing PR is more difficult (it includes many more commits), also there is a risk that someone will press "merge" button and merge it directly into main.

marijanp commented 4 months ago

@koxu1996 You are able to change the base yourself. An accidental merge was not possible now until I actually changed the base because there was another approver missing. A merge of this would also include all the changes from @jonas089 branch.

koxu1996 commented 4 months ago

@koxu1996 You are able to change the base yourself. An accidental merge was not possible now until I actually changed the base because there was another approver missing. A merge of this would also include all the changes from @jonas089 branch.

@marijanp Well... I think we should take responsibility for our own work - I am glad you changed the base. The approver was missing, because I didn't want to let you merge it directly to main :wink:. Even though it includes @jonas089's work, I think Jonas’s PR should be approved and merged - not closed and superseded by this "refactoring" PR.

jonas089 commented 4 months ago

you can merge this, lgtm

edit: I'm done for today and will be back tomorrow morning.

jonas089 commented 4 months ago

I resolved the conflicts and the PR is now ready to be merged.

jonas089 commented 4 months ago

Is this PR ready to be merged?

marijanp commented 4 months ago

@jonas089 not yet, I'm investigating why the GHA fails