Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add public function to retrieve all contributions #76

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented Sep 3, 2024

(Builds on top of #75; only the final commit here is relevant to this PR)

This adds a function to get all contributors and contributions in one-shot. Without this function there's no nice, reliable way to get the current contributors:

  1. you have to fetch the length, then loop to fetch the next address, then fetch the next contribution, and so on. This isn't nice for the external user.

  2. the approach in 1) isn't atomic and so if any mutating contract call happens while iterating you will end up with a wrong (not just outdated) result.

This removes the requirement that new contributions from existing
contributors satisfy the minimum staking requirement.  Such contributors
will already satisfy the minimum requirement (because it would have been
checked by the initial contribution), and so this doesn't need to be
checked.  *Not* checking it allows contributors to increase their stake
in a pending SN with any amount they like.

The current contract behaviour was mimicking the Oxen behaviour which
*does* impose the minimum on top-ups, but that was because of a
implementation detail (realistically, a limitation, perhaps
categorizable as a bug) in Oxen where the minimum was defined not by the
number of distinct contributors, but rather than the number of locked
key images which always increased with each top-up, and so Oxen top-ups
really did take up a contribution spot.
Various packages were complaining about being outdated in various
places; this updates the offenders.
It's not clear how this was *ever* working because the default test
path is `./test/**/*.js`, which was picking up
`test/cpp/external/ethyl/external/oxen-logging/fmt/doc/_static/bootstrap.min.js`
which failed to compile/run with:

    An unexpected error occurred:

    Error: Bootstrap's JavaScript requires jQuery
        at Object.<anonymous> (/home/jagerman/src/eth-sn-contracts/test/cpp/external/ethyl/external/oxen-logging/fmt/doc/_static/bootstrap.min.js:6:37)
        at Module._compile (node:internal/modules/cjs/loader:1364:14)
        at Object.Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
        at Module.load (node:internal/modules/cjs/loader:1203:32)
        at Function.Module._load (node:internal/modules/cjs/loader:1019:12)
        at Module.require (node:internal/modules/cjs/loader:1231:19)
        at require (node:internal/modules/helpers:177:18)
        at /home/jagerman/src/eth-sn-contracts/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/mocha.js:414:36
        at Array.forEach (<anonymous>)
        at Mocha.loadFiles (/home/jagerman/src/eth-sn-contracts/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/mocha.js:411:14)
     ELIFECYCLE  Test failed. See above for more details.

This fixes it to only include test/unit-js  when looking for hardhat
tests to run.
Without this function there's no nice, reliable way to get the current
contributors:

1) you have to fetch the length, then loop to fetch the next address,
   then fetch the next contribution, and so on.  This isn't nice for the
   external user.

2) the approach in 1) isn't atomic and so if any mutating contract call
   happens while iterating you will end up with a wrong (not just
   outdated) result.
@Doy-lee
Copy link
Collaborator

Doy-lee commented Sep 11, 2024

Also LGTM.

@Doy-lee Doy-lee merged commit 6cfcf01 into oxen-io:master Sep 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants